[PATCH v2 0/7] Improve boot command line handling

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

Christophe Leroy (7):
  cmdline: Add generic function to build command line.
  drivers: of: use cmdline building function
  powerpc: convert to generic builtin command line
  cmdline: Add capability to prepend the command line
  powerpc: add capability to prepend default command line
  cmdline: Gives architectures opportunity to use generically defined
    boot cmdline manipulation
  powerpc: use generic CMDLINE manipulations

 arch/powerpc/Kconfig            | 37 ++-----------------
 arch/powerpc/kernel/prom_init.c | 35 +++---------------
 drivers/of/fdt.c                | 23 ++----------
 include/linux/cmdline.h         | 65 +++++++++++++++++++++++++++++++++
 init/Kconfig                    | 56 ++++++++++++++++++++++++++++
 5 files changed, 133 insertions(+), 83 deletions(-)
 create mode 100644 include/linux/cmdline.h

-- 
2.25.0

Most architectures have similar boot command line manipulation
options. This patchs adds the definition in init/Kconfig, gated by
CONFIG_HAVE_CMDLINE that the architectures can select to use them.

In order to use this, a few architectures will have to change their
CONFIG options:
- riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
- architectures using CONFIG_CMDLINE_OVERRIDE or
CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.

Architectures also have to define CONFIG_DEFAULT_CMDLINE.

Signed-off-by: Christophe Leroy <redacted>
---
 init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 22946fe5ded9..a0f2ad9467df 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
 	  Maximum of each of the number of arguments and environment
 	  variables passed to init from the kernel command line.
 
+config HAVE_CMDLINE
+	bool
+
+config CMDLINE_BOOL
+	bool "Default bootloader kernel arguments"
+	depends on HAVE_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.
+
+config CMDLINE
+	string "Initial kernel command string"
+	depends on CMDLINE_BOOL
+	default DEFAULT_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.
+
+choice
+	prompt "Kernel command line type" if CMDLINE != ""
+	default CMDLINE_FROM_BOOTLOADER
+	help
+	  Selects the way you want to use the default kernel arguments.
+
+config CMDLINE_FROM_BOOTLOADER
+	bool "Use bootloader kernel arguments if available"
+	help
+	  Uses the command-line options passed by the boot loader. If
+	  the boot loader doesn't provide any, the default kernel command
+	  string provided in CMDLINE will be used.
+
+config CMDLINE_EXTEND
+	bool "Extend bootloader kernel arguments"
+	help
+	  The default kernel command string will be appended to the
+	  command-line arguments provided during boot.
+
+config CMDLINE_PREPEND
+	bool "Prepend bootloader kernel arguments"
+	help
+	  The default kernel command string will be prepend to the
+	  command-line arguments provided during boot.
+
+config CMDLINE_FORCE
+	bool "Always use the default kernel command string"
+	help
+	  Always use the default kernel command string, even if the boot
+	  loader passes other arguments to the kernel.
+	  This is useful if you cannot or don't want to change the
+	  command-line options your boot loader passes to the kernel.
+endchoice
+
 config COMPILE_TEST
 	bool "Compile also drivers which will not load"
 	depends on !UML && !S390

-- 
2.25.0

This patch activates the capability to prepend default
arguments to the command line.

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/Kconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 386ae12d8523..0ab406f14513 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -912,6 +912,12 @@ config CMDLINE_EXTEND
 	  The command-line arguments provided by the boot loader will be
 	  appended to the default kernel command string.
 
+config CMDLINE_PREPEND
+	bool "Prepend bootloader kernel arguments"
+	help
+	  The default kernel command string will be prepend to the
+	  command-line arguments provided during boot.
+
 config CMDLINE_FORCE
 	bool "Always use the default kernel command string"
 	help

-- 
2.25.0

This patch moves powerpc to the centraly defined CMDLINE options.

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/Kconfig | 43 +++----------------------------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 0ab406f14513..0e1736a2a621 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -195,6 +195,7 @@ config PPC
 	select HAVE_CBPF_JIT			if !PPC64
 	select HAVE_STACKPROTECTOR		if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13)
 	select HAVE_STACKPROTECTOR		if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2)
+	select HAVE_CMDLINE
 	select HAVE_CONTEXT_TRACKING		if PPC64
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DEBUG_STACKOVERFLOW

@@ -886,47 +887,9 @@ config PPC_DENORMALISATION
 	  Add support for handling denormalisation of single precision
 	  values.  Useful for bare metal only.  If unsure say Y here.
 
-config CMDLINE
-	string "Initial kernel command string"
+config DEFAULT_CMDLINE
+	string
 	default ""
-	help
-	  On some platforms, there is currently no way for the boot loader to
-	  pass arguments to the kernel. For these platforms, you can supply
-	  some command-line options at build time by entering them here.  In
-	  most cases you will need to specify the root device here.
-
-choice
-	prompt "Kernel command line type" if CMDLINE != ""
-	default CMDLINE_FROM_BOOTLOADER
-
-config CMDLINE_FROM_BOOTLOADER
-	bool "Use bootloader kernel arguments if available"
-	help
-	  Uses the command-line options passed by the boot loader. If
-	  the boot loader doesn't provide any, the default kernel command
-	  string provided in CMDLINE will be used.
-
-config CMDLINE_EXTEND
-	bool "Extend bootloader kernel arguments"
-	help
-	  The command-line arguments provided by the boot loader will be
-	  appended to the default kernel command string.
-
-config CMDLINE_PREPEND
-	bool "Prepend bootloader kernel arguments"
-	help
-	  The default kernel command string will be prepend to the
-	  command-line arguments provided during boot.
-
-config CMDLINE_FORCE
-	bool "Always use the default kernel command string"
-	help
-	  Always use the default kernel command string, even if the boot
-	  loader passes other arguments to the kernel.
-	  This is useful if you cannot or don't want to change the
-	  command-line options your boot loader passes to the kernel.
-
-endchoice
 
 config EXTRA_TARGETS
 	string "Additional default image types"

-- 
2.25.0

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Daniel

Le 02/03/2021 à 18:35, Daniel Walker a écrit :

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

quoted

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Cool, I'm eager to see them.

Christophe

This patch uses the new cmdline building function to
concatenate the of provided cmdline with built-in parts
based on compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
 drivers/of/fdt.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index dcc1dd96911a..cf2b95b8f298 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c

@@ -25,6 +25,7 @@
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
 #include <linux/random.h>
+#include <linux/cmdline.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>

@@ -1050,26 +1051,10 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 
 	/* Retrieve command line */
 	p = of_get_flat_dt_prop(node, "bootargs", &l);
-	if (p != NULL && l > 0)
-		strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
+	if (l <= 0)
+		p = NULL;
 
-	/*
-	 * CONFIG_CMDLINE is meant to be a default in case nothing else
-	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
-	 * is set in which case we override whatever was found earlier.
-	 */
-#ifdef CONFIG_CMDLINE
-#if defined(CONFIG_CMDLINE_EXTEND)
-	strlcat(data, " ", COMMAND_LINE_SIZE);
-	strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
-	strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#else
-	/* No arguments from boot loader, use kernel's  cmdl*/
-	if (!((char *)data)[0])
-		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#endif
-#endif /* CONFIG_CMDLINE */
+	cmdline_build(data, p, COMMAND_LINE_SIZE);
 
 	pr_debug("Command line is: %s\n", (char *)data);
 

-- 
2.25.0

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
 include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}
+
+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;
+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif
+	if (dest != src)
+		cmdline_strlcat(dest, src, length);
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
+#endif
+}
+
+#endif /* _LINUX_CMDLINE_H */

-- 
2.25.0

This patchs adds an option of prepend a text to the command
line instead of appending it.

Signed-off-by: Christophe Leroy <redacted>
---
 include/linux/cmdline.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
index ae3610bb0ee2..144346051e01 100644
--- a/include/linux/cmdline.h
+++ b/include/linux/cmdline.h

@@ -31,7 +31,7 @@ static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_
 }
 
 /*
- * This function will append a builtin command line to the command
+ * This function will append or prepend a builtin command line to the command
  * line provided by the bootloader. Kconfig options can be used to alter
  * the behavior of this builtin command line.
  * @dest: The destination of the final appended/prepended string.

@@ -50,6 +50,9 @@ static __always_inline void cmdline_build(char *dest, const char *src, size_t le
 		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
 		return;
 	}
+
+	if (IS_ENABLED(CONFIG_CMDLINE_PREPEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, CONFIG_CMDLINE " ", length);
 #endif
 	if (dest != src)
 		cmdline_strlcat(dest, src, length);

-- 
2.25.0

This updates the powerpc code to use the new cmdline building function.

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/kernel/prom_init.c | 35 +++++----------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index ccf77b985c8f..24157e526f80 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -27,6 +27,7 @@
 #include <linux/initrd.h>
 #include <linux/bitops.h>
 #include <linux/pgtable.h>
+#include <linux/cmdline.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
 #include <asm/page.h>

@@ -152,7 +153,7 @@ static struct prom_t __prombss prom;
 static unsigned long __prombss prom_entry;
 
 static char __prombss of_stdout_device[256];
-static char __prombss prom_scratch[256];
+static char __prombss prom_scratch[COMMAND_LINE_SIZE];
 
 static unsigned long __prombss dt_header_start;
 static unsigned long __prombss dt_struct_start, dt_struct_end;

@@ -304,26 +305,6 @@ static char __init *prom_strstr(const char *s1, const char *s2)
 	return NULL;
 }
 
-static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
-{
-	size_t dsize = prom_strlen(dest);
-	size_t len = prom_strlen(src);
-	size_t res = dsize + len;
-
-	/* This would be a bug */
-	if (dsize >= count)
-		return count;
-
-	dest += dsize;
-	count -= dsize;
-	if (len >= count)
-		len = count-1;
-	memcpy(dest, src, len);
-	dest[len] = 0;
-	return res;
-
-}
-
 #ifdef CONFIG_PPC_PSERIES
 static int __init prom_strtobool(const char *s, bool *res)
 {

@@ -768,19 +749,13 @@ static unsigned long prom_memparse(const char *ptr, const char **retptr)
 static void __init early_cmdline_parse(void)
 {
 	const char *opt;
-
-	char *p;
 	int l = 0;
 
-	prom_cmd_line[0] = 0;
-	p = prom_cmd_line;
-
 	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
-		l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
+		l = prom_getprop(prom.chosen, "bootargs", prom_scratch,
+				 COMMAND_LINE_SIZE - 1);
 
-	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
-		prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
-			     sizeof(prom_cmd_line));
+	cmdline_build(prom_cmd_line, l > 0 ? prom_scratch : NULL, sizeof(prom_scratch));
 
 	prom_printf("command line: %s\n", prom_cmd_line);
 

-- 
2.25.0

Hi Christophe,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on robh/for-next linus/master v5.12-rc1 next-20210302]
[cannot apply to mpe/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Improve-boot-command-line-handling/20210303-014039
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: sh-randconfig-s031-20210302 (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-241-geaceeafa-dirty
        # https://github.com/0day-ci/linux/commit/edc3f8320d1dcb21a71e4cfdb26a3d2b64215c30
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Improve-boot-command-line-handling/20210303-014039
        git checkout edc3f8320d1dcb21a71e4cfdb26a3d2b64215c30
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All errors (new ones prefixed by >>):

   arch/sh/Kconfig:760:warning: choice value used outside its choice group

quoted

init/Kconfig:142:error: recursive dependency detected!

   init/Kconfig:142: choice <choice> contains symbol CMDLINE
   init/Kconfig:132: symbol CMDLINE depends on CMDLINE_EXTEND
   init/Kconfig:155: symbol CMDLINE_EXTEND is part of choice <choice>
   For a resolution refer to Documentation/kbuild/kconfig-language.rst
   subsection "Kconfig recursive dependency limitations"


vim +142 init/Kconfig

   103	
   104	config BROKEN
   105		bool
   106	
   107	config BROKEN_ON_SMP
   108		bool
   109		depends on BROKEN || !SMP
   110		default y
   111	
   112	config INIT_ENV_ARG_LIMIT
   113		int
   114		default 32 if !UML
   115		default 128 if UML
   116		help
   117		  Maximum of each of the number of arguments and environment
   118		  variables passed to init from the kernel command line.
   119	
   120	config HAVE_CMDLINE
   121		bool
   122	
   123	config CMDLINE_BOOL
   124		bool "Default bootloader kernel arguments"
   125		depends on HAVE_CMDLINE
   126		help
   127		  On some platforms, there is currently no way for the boot loader to
   128		  pass arguments to the kernel. For these platforms, you can supply
   129		  some command-line options at build time by entering them here.  In
   130		  most cases you will need to specify the root device here.
   131	
   132	config CMDLINE
   133		string "Initial kernel command string"
   134		depends on CMDLINE_BOOL
   135		default DEFAULT_CMDLINE
   136		help
   137		  On some platforms, there is currently no way for the boot loader to
   138		  pass arguments to the kernel. For these platforms, you can supply
   139		  some command-line options at build time by entering them here.  In
   140		  most cases you will need to specify the root device here.
   141	
 > 142	choice
   143		prompt "Kernel command line type" if CMDLINE != ""
   144		default CMDLINE_FROM_BOOTLOADER
   145		help
   146		  Selects the way you want to use the default kernel arguments.
   147	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

+Will D

On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker [off-list ref] wrote:

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

quoted

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Not really a helpful comment. What we merge here will be from whomever
is persistent and timely in their efforts. But please, work together
on a common solution.

This one meets my requirements of moving the kconfig and code out of
the arches, supports prepend/append, and is up to date.

Rob

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

Why are these needed instead of using strlen and strlcat directly?

+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;
+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif

CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?

+	if (dest != src)
+		cmdline_strlcat(dest, src, length);
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
+#endif

Likewise, but also I'm not sure why the sizeof() is required.

Will

On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote:

+Will D

On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker [off-list ref] wrote:

quoted

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

quoted

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Not really a helpful comment. What we merge here will be from whomever
is persistent and timely in their efforts. But please, work together
on a common solution.

This one meets my requirements of moving the kconfig and code out of
the arches, supports prepend/append, and is up to date.

Maintainers are capable of merging whatever they want to merge. However, I
wouldn't make hasty choices. The changes I've been submitting have been deployed
on millions of router instances and are more feature rich.

I believe I worked with you on this change, or something like it,

https://lkml.org/lkml/2019/3/19/970

I don't think Christophe has even addressed this. I've converted many
architectures, and Cisco uses my changes on at least 4 different
architecture. With products deployed and tested.

I will resubmit my changes as soon as I can.

Daniel

On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:

Le 03/03/2021 à 18:28, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
  include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
  1 file changed, 62 insertions(+)
  create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very
early in the boot and KASAN shadow memory is not set up yet so calling
generic string functions would crash the board.

Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.

quoted

quoted

+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;
+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif

CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?

Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it
is feasible. I can change that now.

quoted

quoted

+	if (dest != src)
+		cmdline_strlcat(dest, src, length);
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
+#endif

Likewise, but also I'm not sure why the sizeof() is required.

It is to avoid adding a white space at the end of the command line when
CONFIG_CMDLINE is empty. But maybe it doesn't matter ?

If CONFIG_CMDLINE is empty, I don't think you can select
CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters).

Will

Le 03/03/2021 à 18:46, Will Deacon a écrit :

On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:28, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
   include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
   1 file changed, 62 insertions(+)
   create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very
early in the boot and KASAN shadow memory is not set up yet so calling
generic string functions would crash the board.

Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.

I don't think it is possible to setup an earlier early shadow.

At the point we are in prom_init, the code is not yet relocated at the address it was linked for, 
and it is running with the MMU set by the bootloader, I can't imagine being able to setup MMU 
entries for the early shadow KASAN yet without breaking everything.

Is it really worth trying to point to the normal library routines by default ? It is really only a 
few lines of code hence only not many bytes, and anyway they are in __init section so they get 
discarded at the end.

quoted

quoted

quoted

+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;
+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif

CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?

Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it
is feasible. I can change that now.

quoted

quoted

+	if (dest != src)
+		cmdline_strlcat(dest, src, length);
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
+#endif

Likewise, but also I'm not sure why the sizeof() is required.

It is to avoid adding a white space at the end of the command line when
CONFIG_CMDLINE is empty. But maybe it doesn't matter ?

If CONFIG_CMDLINE is empty, I don't think you can select
CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters).

Ok I'll simplify that when I re-spin.

Christophe

On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:

Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.

+config CMDLINE
+	string "Initial kernel command string"

s/Initial/Default

which is then consistent with the rest of the text here.

+	depends on CMDLINE_BOOL

Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.

+	default DEFAULT_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

(same stale text)

+choice
+	prompt "Kernel command line type" if CMDLINE != ""
+	default CMDLINE_FROM_BOOTLOADER
+	help
+	  Selects the way you want to use the default kernel arguments.

How about:

"Determines how the default kernel arguments are combined with any
 arguments passed by the bootloader"

+config CMDLINE_FROM_BOOTLOADER
+	bool "Use bootloader kernel arguments if available"
+	help
+	  Uses the command-line options passed by the boot loader. If
+	  the boot loader doesn't provide any, the default kernel command
+	  string provided in CMDLINE will be used.
+
+config CMDLINE_EXTEND

Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.

+	bool "Extend bootloader kernel arguments"

"Append to the bootloader kernel arguments"

+	help
+	  The default kernel command string will be appended to the
+	  command-line arguments provided during boot.

s/provided during boot/provided by the bootloader/

+
+config CMDLINE_PREPEND
+	bool "Prepend bootloader kernel arguments"

"Prepend to the bootloader kernel arguments"

+	help
+	  The default kernel command string will be prepend to the
+	  command-line arguments provided during boot.

s/prepend/prepended/
s/provided during boot/provided by the bootloader/

+
+config CMDLINE_FORCE
+	bool "Always use the default kernel command string"
+	help
+	  Always use the default kernel command string, even if the boot
+	  loader passes other arguments to the kernel.
+	  This is useful if you cannot or don't want to change the
+	  command-line options your boot loader passes to the kernel.

I find the "This is useful if ..." sentence really confusing, perhaps just
remove it? I'd then tweak it to be:

  "Always use the default kernel command string, ignoring any arguments
   provided by the bootloader."

Will

Le 03/03/2021 à 18:39, Daniel Walker a écrit :

On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote:

quoted

+Will D

On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker [off-list ref] wrote:

quoted

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

quoted

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Not really a helpful comment. What we merge here will be from whomever
is persistent and timely in their efforts. But please, work together
on a common solution.

This one meets my requirements of moving the kconfig and code out of
the arches, supports prepend/append, and is up to date.

Maintainers are capable of merging whatever they want to merge. However, I
wouldn't make hasty choices. The changes I've been submitting have been deployed
on millions of router instances and are more feature rich.

I believe I worked with you on this change, or something like it,

https://lkml.org/lkml/2019/3/19/970

I don't think Christophe has even addressed this.

I thing I have, see 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3b4291271ce4af4941a771e5af5cbba3c8fa1b2a.1614705851.git.christophe.leroy@csgroup.eu/

If you see something missing in that patch, can you tell me.

I've converted many
architectures, and Cisco uses my changes on at least 4 different
architecture. With products deployed and tested.

As far as we know, only powerpc was converted in the last series you submitted, see 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=98106&state=*

I will resubmit my changes as soon as I can.

Christophe

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
 include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 include/linux/cmdline.h

Sorry, spotted a couple of other things...

+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;

length is unsigned

+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif
+	if (dest != src)

The kernel-doc says that @src "Must not equal dest".

Will

On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote:

Le 03/03/2021 à 18:46, Will Deacon a écrit :

quoted

On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:28, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
   include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
   1 file changed, 62 insertions(+)
   create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very
early in the boot and KASAN shadow memory is not set up yet so calling
generic string functions would crash the board.

Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.

I don't think it is possible to setup an earlier early shadow.

At the point we are in prom_init, the code is not yet relocated at the
address it was linked for, and it is running with the MMU set by the
bootloader, I can't imagine being able to setup MMU entries for the early
shadow KASAN yet without breaking everything.

That's very similar to us; we're not relocated, although we are at least
in control of the MMU (which is using a temporary set of page-tables).

Is it really worth trying to point to the normal library routines by default
? It is really only a few lines of code hence only not many bytes, and
anyway they are in __init section so they get discarded at the end.

I would prefer to use the normal routines by default and allow architectures
to override them based on their needs, otherwise we'll end up trying to
maintain a "lowest-common-dominator" set of string routines that can be
safely run in whatever different constraints different architectures have.

Will

On Tue, Mar 02, 2021 at 05:25:20PM +0000, Christophe Leroy wrote:

Same comment as the other patch: I don't think we need to worry about the
sizeof() here.

Will

Le 03/03/2021 à 18:28, Will Deacon a écrit :

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
  include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
  1 file changed, 62 insertions(+)
  create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very early in the boot and KASAN 
shadow memory is not set up yet so calling generic string functions would crash the board.

quoted

+/*
+ * This function will append a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string.
+ * @src: The starting string or NULL if there isn't one. Must not equal dest.
+ * @length: the length of dest buffer.
+ */
+static __always_inline void cmdline_build(char *dest, const char *src, size_t length)
+{
+	if (length <= 0)
+		return;
+
+	dest[0] = 0;
+
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) {
+		cmdline_strlcat(dest, CONFIG_CMDLINE, length);
+		return;
+	}
+#endif

CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't,
CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef?

Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it is feasible. I can 
change that now.

quoted

+	if (dest != src)
+		cmdline_strlcat(dest, src, length);
+#ifdef CONFIG_CMDLINE
+	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1)
+		cmdline_strlcat(dest, " " CONFIG_CMDLINE, length);
+#endif

Likewise, but also I'm not sure why the sizeof() is required.

It is to avoid adding a white space at the end of the command line when CONFIG_CMDLINE is empty. But 
maybe it doesn't matter ?

Christophe

On Wed, Mar 03, 2021 at 07:07:45PM +0100, Christophe Leroy wrote:

Le 03/03/2021 à 18:39, Daniel Walker a écrit :

quoted

On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote:

quoted

+Will D

On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker [off-list ref] wrote:

quoted

On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote:

quoted

The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

It is first focussed on powerpc but also extends the capability
for other arches.

This is based on suggestion from Daniel Walker [off-list ref]

I don't see a point in your changes at this time. My changes are much more
mature, and you changes don't really make improvements.

Not really a helpful comment. What we merge here will be from whomever
is persistent and timely in their efforts. But please, work together
on a common solution.

This one meets my requirements of moving the kconfig and code out of
the arches, supports prepend/append, and is up to date.

Maintainers are capable of merging whatever they want to merge. However, I
wouldn't make hasty choices. The changes I've been submitting have been deployed
on millions of router instances and are more feature rich.

I believe I worked with you on this change, or something like it,

https://lkml.org/lkml/2019/3/19/970

I don't think Christophe has even addressed this.

I thing I have, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3b4291271ce4af4941a771e5af5cbba3c8fa1b2a.1614705851.git.christophe.leroy@csgroup.eu/

If you see something missing in that patch, can you tell me.

 
Ok, must have missed that one.

quoted

I've converted many
architectures, and Cisco uses my changes on at least 4 different
architecture. With products deployed and tested.

As far as we know, only powerpc was converted in the last series you
submitted, see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=98106&state=*

Me and others submitted my changes many times, and other architectures have been included. The patch
you submitted I've submitted similar at Rob's request years ago.

Here a fuller submissions some time ago,

https://lore.kernel.org/patchwork/cover/992768/

You've only been involved in prior powerpc only submissions.

Daniel

Will Deacon [off-list ref] writes:

On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:46, Will Deacon a écrit :

quoted

On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:28, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
   include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
   1 file changed, 62 insertions(+)
   create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very
early in the boot and KASAN shadow memory is not set up yet so calling
generic string functions would crash the board.

Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.

I don't think it is possible to setup an earlier early shadow.

At the point we are in prom_init, the code is not yet relocated at the
address it was linked for, and it is running with the MMU set by the
bootloader, I can't imagine being able to setup MMU entries for the early
shadow KASAN yet without breaking everything.

That's very similar to us; we're not relocated, although we are at least
in control of the MMU (which is using a temporary set of page-tables).

prom_init runs as an OF client, with the MMU off (except on some Apple
machines), and we don't own the MMU. So there's really nothing we can do :)

Though now that I look at it, I don't think we should be doing this
level of commandline handling in prom_init. It should just grab the
value from firmware and pass it to the kernel proper, and then all the
prepend/append/force etc. logic should happen there.

cheers

Le 05/03/2021 à 12:58, Michael Ellerman a écrit :

Will Deacon [off-list ref] writes:

quoted

On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:46, Will Deacon a écrit :

quoted

On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote:

quoted

Le 03/03/2021 à 18:28, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote:

quoted

This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.

Signed-off-by: Christophe Leroy <redacted>
---
    include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++
    1 file changed, 62 insertions(+)
    create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..ae3610bb0ee2
--- /dev/null
+++ b/include/linux/cmdline.h

@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+static __always_inline size_t cmdline_strlen(const char *s)
+{
+	const char *sc;
+
+	for (sc = s; *sc != '\0'; ++sc)
+		; /* nothing */
+	return sc - s;
+}
+
+static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count)
+{
+	size_t dsize = cmdline_strlen(dest);
+	size_t len = cmdline_strlen(src);
+	size_t res = dsize + len;
+
+	/* This would be a bug */
+	if (dsize >= count)
+		return count;
+
+	dest += dsize;
+	count -= dsize;
+	if (len >= count)
+		len = count - 1;
+	memcpy(dest, src, len);
+	dest[len] = 0;
+	return res;
+}

Why are these needed instead of using strlen and strlcat directly?

Because on powerpc (at least), it will be used in prom_init, it is very
early in the boot and KASAN shadow memory is not set up yet so calling
generic string functions would crash the board.

Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can
you not do something similar? Failing that, I think it would be better to
offer the option for an arch to implement cmdline_*, but have then point to
the normal library routines by default.

I don't think it is possible to setup an earlier early shadow.

At the point we are in prom_init, the code is not yet relocated at the
address it was linked for, and it is running with the MMU set by the
bootloader, I can't imagine being able to setup MMU entries for the early
shadow KASAN yet without breaking everything.

That's very similar to us; we're not relocated, although we are at least
in control of the MMU (which is using a temporary set of page-tables).

prom_init runs as an OF client, with the MMU off (except on some Apple
machines), and we don't own the MMU. So there's really nothing we can do :)

Though now that I look at it, I don't think we should be doing this
level of commandline handling in prom_init. It should just grab the
value from firmware and pass it to the kernel proper, and then all the
prepend/append/force etc. logic should happen there.

But then, how do you handle the command line parameters that are needed by prom_init ?

For instance, prom_init_mem() use 'prom_memory_limit', which comes from the "mem=" option in the 
command line.

Christophe

On Fri, Mar 05, 2021 at 10:58:02PM +1100, Michael Ellerman wrote:

Will Deacon [off-list ref] writes:

quoted

That's very similar to us; we're not relocated, although we are at least
in control of the MMU (which is using a temporary set of page-tables).

prom_init runs as an OF client, with the MMU off (except on some Apple
machines), and we don't own the MMU. So there's really nothing we can do :)

You *could* take over all memory mapping.  This is complex, and I
estimate the change you get this to work correctly on all supported
systems to be between -400% and 0%.

And not very long later Linux jettisons OF completely anyway.

Though now that I look at it, I don't think we should be doing this
level of commandline handling in prom_init. It should just grab the
value from firmware and pass it to the kernel proper, and then all the
prepend/append/force etc. logic should happen there.

That sounds much simpler, yes :-)


Segher

On Fri, Mar 05, 2021 at 01:49:03PM +0100, Christophe Leroy wrote:

Le 05/03/2021 à 12:58, Michael Ellerman a écrit :

quoted

prom_init runs as an OF client, with the MMU off (except on some Apple
machines), and we don't own the MMU. So there's really nothing we can do :)

Though now that I look at it, I don't think we should be doing this
level of commandline handling in prom_init. It should just grab the
value from firmware and pass it to the kernel proper, and then all the
prepend/append/force etc. logic should happen there.

But then, how do you handle the command line parameters that are needed by 
prom_init ?

For instance, prom_init_mem() use 'prom_memory_limit', which comes from the 
"mem=" option in the command line.

*Reading* it is easy, much easier than modifying it.


Segher

Le 03/03/2021 à 18:57, Will Deacon a écrit :

On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:

quoted

Most architectures have similar boot command line manipulation
options. This patchs adds the definition in init/Kconfig, gated by
CONFIG_HAVE_CMDLINE that the architectures can select to use them.

In order to use this, a few architectures will have to change their
CONFIG options:
- riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
- architectures using CONFIG_CMDLINE_OVERRIDE or
CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.

Architectures also have to define CONFIG_DEFAULT_CMDLINE.

Signed-off-by: Christophe Leroy <redacted>
---
  init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 56 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 22946fe5ded9..a0f2ad9467df 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
  	  Maximum of each of the number of arguments and environment
  	  variables passed to init from the kernel command line.
  
+config HAVE_CMDLINE
+	bool
+
+config CMDLINE_BOOL
+	bool "Default bootloader kernel arguments"
+	depends on HAVE_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.

quoted

+config CMDLINE
+	string "Initial kernel command string"

s/Initial/Default

which is then consistent with the rest of the text here.

quoted

+	depends on CMDLINE_BOOL

Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.

quoted

+	default DEFAULT_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

(same stale text)

quoted

+choice
+	prompt "Kernel command line type" if CMDLINE != ""
+	default CMDLINE_FROM_BOOTLOADER
+	help
+	  Selects the way you want to use the default kernel arguments.

How about:

"Determines how the default kernel arguments are combined with any
  arguments passed by the bootloader"

quoted

+config CMDLINE_FROM_BOOTLOADER
+	bool "Use bootloader kernel arguments if available"
+	help
+	  Uses the command-line options passed by the boot loader. If
+	  the boot loader doesn't provide any, the default kernel command
+	  string provided in CMDLINE will be used.
+
+config CMDLINE_EXTEND

Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.

Argh, yes. Seems like the problem is even larger than that IIUC:

- For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE
- For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments
- For SH  it means to append the CONFIG_CMDLINE to the bootloader arguments
- For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE
- For OF it means to append the CONFIG_CMDLINE to the bootloader arguments

So what happens on ARM for instance when it selects CONFIG_OF for instance ?
Or should we consider that EXTEND means APPEND or PREPEND, no matter which ?
Because EXTEND is for instance used for:

	config INITRAMFS_FORCE
		bool "Ignore the initramfs passed by the bootloader"
		depends on CMDLINE_EXTEND || CMDLINE_FORCE


Christophe

On Thu, Mar 25, 2021 at 12:18:38PM +0100, Christophe Leroy wrote:

Le 03/03/2021 à 18:57, Will Deacon a écrit :

quoted

On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:

quoted

Most architectures have similar boot command line manipulation
options. This patchs adds the definition in init/Kconfig, gated by
CONFIG_HAVE_CMDLINE that the architectures can select to use them.

In order to use this, a few architectures will have to change their
CONFIG options:
- riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
- architectures using CONFIG_CMDLINE_OVERRIDE or
CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.

Architectures also have to define CONFIG_DEFAULT_CMDLINE.

Signed-off-by: Christophe Leroy <redacted>
---
  init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 56 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 22946fe5ded9..a0f2ad9467df 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
  	  Maximum of each of the number of arguments and environment
  	  variables passed to init from the kernel command line.
+config HAVE_CMDLINE
+	bool
+
+config CMDLINE_BOOL
+	bool "Default bootloader kernel arguments"
+	depends on HAVE_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.

quoted

+config CMDLINE
+	string "Initial kernel command string"

s/Initial/Default

which is then consistent with the rest of the text here.

quoted

+	depends on CMDLINE_BOOL

Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.

quoted

+	default DEFAULT_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

(same stale text)

quoted

+choice
+	prompt "Kernel command line type" if CMDLINE != ""
+	default CMDLINE_FROM_BOOTLOADER
+	help
+	  Selects the way you want to use the default kernel arguments.

How about:

"Determines how the default kernel arguments are combined with any
  arguments passed by the bootloader"

quoted

+config CMDLINE_FROM_BOOTLOADER
+	bool "Use bootloader kernel arguments if available"
+	help
+	  Uses the command-line options passed by the boot loader. If
+	  the boot loader doesn't provide any, the default kernel command
+	  string provided in CMDLINE will be used.
+
+config CMDLINE_EXTEND

Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.

Argh, yes. Seems like the problem is even larger than that IIUC:

- For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE
- For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments
- For SH  it means to append the CONFIG_CMDLINE to the bootloader arguments
- For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE
- For OF it means to append the CONFIG_CMDLINE to the bootloader arguments

So what happens on ARM for instance when it selects CONFIG_OF for instance ?

I think ARM gets different behaviour depending on whether it uses ATAGs or
FDT.

Or should we consider that EXTEND means APPEND or PREPEND, no matter which ?
Because EXTEND is for instance used for:

	config INITRAMFS_FORCE
		bool "Ignore the initramfs passed by the bootloader"
		depends on CMDLINE_EXTEND || CMDLINE_FORCE

Oh man, I didn't spot that one :(

I think I would make the generic options explicit: either APPEND or PREPEND.
Then architectures which choose to define CMDLINE_EXTEND in their Kconfigs
can select the generic option that matches their behaviour.

INITRAMFS_FORCE sounds like it should depend on APPEND (assuming that means
CONFIG_CMDLINE is appended to the bootloader arguments).

Will

Le 25/03/2021 à 20:32, Will Deacon a écrit :

On Thu, Mar 25, 2021 at 12:18:38PM +0100, Christophe Leroy wrote:

quoted

- For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE
- For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments
- For SH  it means to append the CONFIG_CMDLINE to the bootloader arguments
- For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE
- For OF it means to append the CONFIG_CMDLINE to the bootloader arguments

So what happens on ARM for instance when it selects CONFIG_OF for instance ?

I think ARM gets different behaviour depending on whether it uses ATAGs or
FDT.

As far as I can see, ARM uses either ATAGs only or both ATAGs and FDT. ATAGs is forced to 'y' when 
USE_OF is set. Do I miss something ?

quoted

Or should we consider that EXTEND means APPEND or PREPEND, no matter which ?
Because EXTEND is for instance used for:

	config INITRAMFS_FORCE
		bool "Ignore the initramfs passed by the bootloader"
		depends on CMDLINE_EXTEND || CMDLINE_FORCE

Oh man, I didn't spot that one :(

I think I would make the generic options explicit: either APPEND or PREPEND.
Then architectures which choose to define CMDLINE_EXTEND in their Kconfigs
can select the generic option that matches their behaviour.

INITRAMFS_FORCE sounds like it should depend on APPEND (assuming that means
CONFIG_CMDLINE is appended to the bootloader arguments).

Christophe

Le 03/03/2021 à 18:57, Will Deacon a écrit :

On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:

quoted

Most architectures have similar boot command line manipulation
options. This patchs adds the definition in init/Kconfig, gated by
CONFIG_HAVE_CMDLINE that the architectures can select to use them.

In order to use this, a few architectures will have to change their
CONFIG options:
- riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
- architectures using CONFIG_CMDLINE_OVERRIDE or
CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.

Architectures also have to define CONFIG_DEFAULT_CMDLINE.

Signed-off-by: Christophe Leroy <redacted>
---
  init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 56 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 22946fe5ded9..a0f2ad9467df 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
  	  Maximum of each of the number of arguments and environment
  	  variables passed to init from the kernel command line.
  
+config HAVE_CMDLINE
+	bool
+
+config CMDLINE_BOOL
+	bool "Default bootloader kernel arguments"
+	depends on HAVE_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.

quoted

+config CMDLINE
+	string "Initial kernel command string"

s/Initial/Default

which is then consistent with the rest of the text here.

quoted

+	depends on CMDLINE_BOOL

Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.

You are right, the help text is duplicated, I will change the text for the CMDLINE_BOOL

quoted

+	default DEFAULT_CMDLINE
+	help
+	  On some platforms, there is currently no way for the boot loader to
+	  pass arguments to the kernel. For these platforms, you can supply
+	  some command-line options at build time by entering them here.  In
+	  most cases you will need to specify the root device here.

(same stale text)

quoted

+choice
+	prompt "Kernel command line type" if CMDLINE != ""
+	default CMDLINE_FROM_BOOTLOADER
+	help
+	  Selects the way you want to use the default kernel arguments.

How about:

"Determines how the default kernel arguments are combined with any
  arguments passed by the bootloader"

quoted

+config CMDLINE_FROM_BOOTLOADER
+	bool "Use bootloader kernel arguments if available"
+	help
+	  Uses the command-line options passed by the boot loader. If
+	  the boot loader doesn't provide any, the default kernel command
+	  string provided in CMDLINE will be used.
+
+config CMDLINE_EXTEND

Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.

quoted

+	bool "Extend bootloader kernel arguments"

"Append to the bootloader kernel arguments"

quoted

+	help
+	  The default kernel command string will be appended to the
+	  command-line arguments provided during boot.

s/provided during boot/provided by the bootloader/

quoted

+
+config CMDLINE_PREPEND
+	bool "Prepend bootloader kernel arguments"

"Prepend to the bootloader kernel arguments"

quoted

+	help
+	  The default kernel command string will be prepend to the
+	  command-line arguments provided during boot.

s/prepend/prepended/
s/provided during boot/provided by the bootloader/

quoted

+
+config CMDLINE_FORCE
+	bool "Always use the default kernel command string"
+	help
+	  Always use the default kernel command string, even if the boot
+	  loader passes other arguments to the kernel.
+	  This is useful if you cannot or don't want to change the
+	  command-line options your boot loader passes to the kernel.

I find the "This is useful if ..." sentence really confusing, perhaps just
remove it? I'd then tweak it to be:

   "Always use the default kernel command string, ignoring any arguments
    provided by the bootloader."

Taken all your suggested text.

Thanks
Christophe