Re: [PATCH 1/8] CMDLINE: add generic builtin command line

[Christophe Leroy]

Le 10/11/2023 à 02:38, Daniel Walker a écrit :
> This code allows architectures to use a generic builtin command line.
> The state of the builtin command line options across architecture is
> diverse. MIPS and X86 once has similar systems, then mips added some
> options to allow extending the command line. Powerpc did something
> simiar in adding the ability to extend. Even with mips and powerpc
> enhancement the needs of Cisco are not met on these platforms.

What are those needs, can you list them in the cover letter, and 
probably also in here ?
Are those needs specific to Cisco or can they interest the entire Linux 
community ?

> 
> The code in this commit unifies the code into a generic
> header file under the CONFIG_GENERIC_CMDLINE option. When this
> option is enabled the architecture can call the cmdline_add_builtin()
> to add the builtin command line. The generic code provides both
> append and/or prepend options and provides a way to redefine these
> option after the kernel is compiled.

Explain how.

> 
> This code also includes test's which are meant to confirm
> functionality.

Would be better to have test part as a separate patch if possible.

> 
> This unified implementation offers the same functionality needed by
> Cisco on all platform which we enable it on.

Cisco ... cisco ... cisco ...

> 
> Cc: xe-linux-external@xxxxxxxxx
> Signed-off-by: Ruslan Bilovol <rbilovol@xxxxxxxxx>
> Signed-off-by: Daniel Walker <danielwa@xxxxxxxxx>
> ---
>   include/linux/cmdline.h | 106 ++++++++++++++++++++++++++++++
>   init/Kconfig            |  79 +++++++++++++++++++++++
>   lib/Kconfig             |   4 ++
>   lib/Makefile            |   3 +
>   lib/generic_cmdline.S   |  53 +++++++++++++++
>   lib/test_cmdline1.c     | 139 ++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 384 insertions(+)
>   create mode 100644 include/linux/cmdline.h
>   create mode 100644 lib/generic_cmdline.S
>   create mode 100644 lib/test_cmdline1.c
> 
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..a94758a0f257
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_CMDLINE_H
> +#define _LINUX_CMDLINE_H
> +/*
> + *
> + * Copyright (C) 2006,2021. Cisco Systems, Inc.
> + *
> + * Generic Append/Prepend cmdline support.
> + */
> +
> +
> +#include <linux/ctype.h>
> +#include <linux/cache.h>
> +#include <asm/setup.h>
> +
> +#ifdef CONFIG_CMDLINE_BOOL
> +extern char cmdline_prepend[];
> +extern char cmdline_append[];
> +extern char cmdline_tmp[];
> +#define CMDLINE_PREPEND cmdline_prepend
> +#define CMDLINE_APPEND cmdline_append
> +#define CMDLINE_TMP cmdline_tmp
> +#define CMDLINE_STATIC_PREPEND CONFIG_CMDLINE_PREPEND
> +#define CMDLINE_STATIC_APPEND CONFIG_CMDLINE_APPEND

Too many macros reduces the readability of code, avoid them when possible.

> +#else

Explain why this else leg is needed. It should be possible to set 
default values directly in Kconfig.

> +#define CMDLINE_PREPEND ""
> +#define CMDLINE_APPEND ""
> +#define CMDLINE_TMP ""
> +#define CMDLINE_STATIC_PREPEND ""
> +#define CMDLINE_STATIC_APPEND ""
> +#endif
> +
> +#ifndef CMDLINE_STRLCAT
> +#define CMDLINE_STRLCAT strlcat
> +#endif
> +
> +#ifndef CMDLINE_STRLEN
> +#define CMDLINE_STRLEN strlen
> +#endif
> +
> +/*
> + * 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
> + * @tmp: temporary space used for prepending
> + * @prepend: string to prepend to @dest
> + * @append: string to append to @dest
> + * @length: the maximum length of the strings above.
> + * @cmdline_strlen: point to a compatible strlen

Remove that function pointer argument and use macros.

> + * @cmdline_strlcat: point to a compatible strlcat

Same

> + * This function returns true when the builtin command line was copied successfully
> + * and false when there was not enough room to copy all parts of the command line.

What happens when it returns false, is it partially copied or nothing is 
done ?

> + */
> +static inline bool
> +__cmdline_add_builtin(
> +		char *dest,
> +		char *tmp,
> +		char *prepend,
> +		char *append,
> +		unsigned long length,
> +		size_t (*cmdline_strlen)(const char *s),
> +		size_t (*cmdline_strlcat)(char *dest, const char *src, size_t count))

This cmdline feature is used in early deep parts of architectures, so in 
a way more or less comparable to linux-mm. Approach with linux-mm has 
always been to define macros that can be overriden by architectures. 
Please do the same and define cmdline_strlen() and cmdline_strlcat() as 
macros that can be overriden by architectures instead of passing 
function pointers. And keep macro names as lower case for this type of 
macros.

> +{
> +	size_t total_length = 0, tmp_length;

Try to use shorter names for local variables, see kernel codying style.

> +
> +	if (!IS_ENABLED(CONFIG_GENERIC_CMDLINE))
> +		return true;
> +
> +	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL))
> +		return true;
> +
> +	if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
> +		dest[0] = '\0';
> +	else
> +		total_length += cmdline_strlen(dest);

All those Kconfig options should be explained in the commit message, 
that would help understanding the patch.

Impact on existing defconfigs should also be taken care of and minimised.

> +
> +	tmp_length = cmdline_strlen(append);
> +	if (tmp_length > 0) {
> +		cmdline_strlcat(dest, append, length);
> +		total_length += tmp_length;
> +	}
> +
> +	tmp_length = cmdline_strlen(prepend);
> +	if (tmp_length > 0) {
> +		cmdline_strlcat(tmp, prepend, length);
> +		cmdline_strlcat(tmp, dest, length);
> +		dest[0] = '\0';
> +		cmdline_strlcat(dest, tmp, length);
> +		total_length += tmp_length;
> +	}
> +
> +	tmp[0] = '\0';

What's the purpose of setting tmp[0] to 0 ?

> +
> +	if (total_length > length)
> +		return false;
> +
> +	return true;

Can be writen as:

	return total_length <= length;

> +}
> +
> +#define cmdline_add_builtin(dest) \
> +	__cmdline_add_builtin(dest, CMDLINE_TMP, CMDLINE_PREPEND, CMDLINE_APPEND, COMMAND_LINE_SIZE, CMDLINE_STRLEN, CMDLINE_STRLCAT)
> +
> +#define cmdline_get_static_builtin(dest) \
> +	(CMDLINE_STATIC_PREPEND CMDLINE_STATIC_APPEND)
> +#endif
> diff --git a/init/Kconfig b/init/Kconfig
> index 6d35728b94b2..703eed88d140 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1922,6 +1922,85 @@ config TRACEPOINTS
>   
>   source "kernel/Kconfig.kexec"
>   
> +config GENERIC_CMDLINE
> +	bool
> +
> +if GENERIC_CMDLINE
> +
> +config CMDLINE_BOOL
> +	bool "Built-in kernel command line"
> +	help
> +	  Allow for specifying boot arguments to the kernel at
> +	  build time.  On some systems (e.g. embedded ones), it is
> +	  necessary or convenient to provide some or all of the
> +	  kernel boot arguments with the kernel itself (that is,
> +	  to not rely on the boot loader to provide them.)
> +
> +	  To compile command line arguments into the kernel,
> +	  set this option to 'Y', then fill in the
> +	  the boot arguments in CONFIG_CMDLINE.
> +
> +	  Systems with fully functional boot loaders (i.e. non-embedded)
> +	  should leave this option set to 'N'.
> +
> +config CMDLINE_APPEND
> +	string "Built-in kernel command string append"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is appended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_PREPEND
> +	string "Built-in kernel command string prepend"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is prepended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_EXTRA
> +	bool "Reserve more space for inserting prepend and append without recompiling"
> +	depends on CMDLINE_BOOL
> +	select SYSTEM_EXTRA_CERTIFICATE
> +	help
> +	  If set, space for an append and prepend will be reserved in the kernel
> +	  image. This allows updating or changing the append and prepend to a large
> +	  string then the kernel was compiled for without recompiling the kernel.

s/large/larger
s/then/than

> +
> +	  The maximum size is the command line size for each prepend and append.
> +
> +config CMDLINE_OVERRIDE
> +	bool "Built-in command line overrides boot loader arguments"
> +	depends on CMDLINE_BOOL
> +	help
> +	  Set this option to 'Y' to have the kernel ignore the boot loader
> +	  command line, and use ONLY the built-in command line. In this case
> +	  append and prepend strings are concatenated to form the full
> +	  command line.
> +
> +	  This is used to work around broken boot loaders.  This should
> +	  be set to 'N' under normal conditions.
> +endif

An analysis of what exists on each existing architecture should 
demonstrate that you have defined all needed options.

> +
>   endmenu		# General setup
>   
>   source "arch/Kconfig"
> diff --git a/lib/Kconfig b/lib/Kconfig
> index c686f4adc124..d520f1aa7c32 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -729,6 +729,10 @@ config PARMAN
>   config OBJAGG
>   	tristate "objagg" if COMPILE_TEST
>   
> +config TEST_CMDLINE
> +	depends on CMDLINE_BOOL && !CMDLINE_OVERRIDE
> +	tristate "Test generic command line handling"
> +

Put the test part in a second patch.

>   endmenu
>   
>   config GENERIC_IOREMAP
> diff --git a/lib/Makefile b/lib/Makefile
> index 740109b6e2c8..aa7b14a0ced7 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -438,3 +438,6 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE
>   ifeq ($(CONFIG_FORTIFY_SOURCE),y)
>   $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)
>   endif
> +
> +obj-$(CONFIG_TEST_CMDLINE) += test_cmdline1.o
> +obj-$(CONFIG_CMDLINE_BOOL)     += generic_cmdline.o
> diff --git a/lib/generic_cmdline.S b/lib/generic_cmdline.S
> new file mode 100644
> index 000000000000..223763f9eeb6
> --- /dev/null
> +++ b/lib/generic_cmdline.S
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/export.h>
> +#include <linux/init.h>
> +
> +#include <asm/setup.h>
> +
> +        __INITDATA
> +
> +       .align 8
> +       .global cmdline_prepend
> +cmdline_prepend:
> +       .ifnc CONFIG_CMDLINE_PREPEND,""
> +       .ascii CONFIG_CMDLINE_PREPEND
> +       .string " "
> +       .else
> +       .byte 0x0
> +       .endif
> +#ifdef CONFIG_CMDLINE_EXTRA
> +       .space COMMAND_LINE_SIZE - (.-cmdline_prepend)
> +       .size cmdline_prepend, COMMAND_LINE_SIZE
> +#endif /* CONFIG_CMDLINE_EXTRA */
> +
> +cmdline_prepend_end:
> +       .size cmdline_prepend, (cmdline_prepend_end - cmdline_prepend)
> +
> +       .align 8
> +       .global cmdline_tmp
> +cmdline_tmp:
> +       .ifnc CONFIG_CMDLINE_PREPEND,""
> +       .size cmdline_tmp, COMMAND_LINE_SIZE
> +       .space COMMAND_LINE_SIZE
> +       .else
> +       .byte 0x0
> +       .endif
> +cmdline_tmp_end:
> +       .size cmdline_tmp, (cmdline_tmp_end - cmdline_tmp)
> +
> +       .align 8
> +       .global cmdline_append
> +       .size cmdline_append, COMMAND_LINE_SIZE
> +cmdline_append:
> +       .ifnc CONFIG_CMDLINE_APPEND,""
> +       .ascii " "
> +       .string CONFIG_CMDLINE_APPEND
> +       .else
> +       .byte 0x0
> +       .endif
> +#ifdef CONFIG_CMDLINE_EXTRA
> +       .space COMMAND_LINE_SIZE - (.-cmdline_append)
> +#endif /* CONFIG_CMDLINE_EXTRA */
> +cmdline_append_end:
> +       .size cmdline_append, (cmdline_append_end - cmdline_append)
> +

Can all this be done in a C file, possibly with inline assembly if 
required, instead of an assembly file ?

> diff --git a/lib/test_cmdline1.c b/lib/test_cmdline1.c
> new file mode 100644
> index 000000000000..bcaffcc024e4
> --- /dev/null
> +++ b/lib/test_cmdline1.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Why 2.0-only ? Not sure it is the preferred licence for new files unless 
they are tied to something already existing.

> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/bitmap.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/cmdline.h>
> +#include <linux/uaccess.h>
> +
> +#include "../tools/testing/selftests/kselftest_module.h"

Is it common practice to include tools/testing/ stuff into kernel ?

> +
> +KSTM_MODULE_GLOBALS();
> +
> +char test1_prepend[] = "prepend ";
> +char test1_append[] = " append";
> +char test1_bootloader_args[] = "console=ttyS0 log_level=3";
> +char test1_result[] = "prepend console=ttyS0 log_level=3 append";
> +
> +char test2_append[] = " append";
> +char test2_bootloader_args[] = "console=ttyS0 log_level=3";
> +char test2_result[] = "console=ttyS0 log_level=3 append";
> +
> +char test3_prepend[] = "prepend ";
> +char test3_bootloader_args[] = "console=ttyS0 log_level=3";
> +char test3_result[] = "prepend console=ttyS0 log_level=3";
> +
> +char test4_bootloader_args[] = "console=ttyS0 log_level=3";
> +char test4_result[] = "console=ttyS0 log_level=3";
> +
> +char test5_prepend[] = "prepend ";
> +char test5_append[] = " append";
> +static char test5_bootloader_args[] =
> +"00000000000000 011111111111111 0222222222222222 033333333333333 "
> +"10000000000000 111111111111111 1222222222222222 133333333333333 "
> +"20000000000000 211111111111111 2222222222222222 233333333333333 "
> +"30000000000000 311111111111111 3222222222222222 333333333333333 "
> +"40000000000000 411111111111111 4222222222222222 433333333333333 "
> +"50000000000000 511111111111111 5222222222222222 533333333333333 "
> +"60000000000000 611111111111111 6222222222222222 633333333333333 "
> +"70000000000000 711111111111111 7222222222222222 733333333333333";
> +static char test5_result[] =
> +"prepend 00000000000000 011111111111111 0222222222222222 033333333333333 "
> +"10000000000000 111111111111111 1222222222222222 133333333333333 "
> +"20000000000000 211111111111111 2222222222222222 233333333333333 "
> +"30000000000000 311111111111111 3222222222222222 333333333333333 "
> +"40000000000000 411111111111111 4222222222222222 433333333333333 "
> +"50000000000000 511111111111111 5222222222222222 533333333333333 "
> +"60000000000000 611111111111111 6222222222222222 633333333333333 "
> +"70000000000000 711111111111111 7222222222222222 7333333";
> +
> +char test5_boot_command_line[COMMAND_LINE_SIZE];
> +
> +char test_tmp[COMMAND_LINE_SIZE];
> +
> +char test_boot_command_line[COMMAND_LINE_SIZE];
> +
> +static void __init selftest(void)
> +{
> +	bool result;
> +
> +	/* Normal operation */
> +	strcpy(test_boot_command_line, test1_bootloader_args);
> +	test_tmp[0] = '\0';
> +	result = __cmdline_add_builtin(test_boot_command_line, test_tmp, test1_prepend, test1_append, COMMAND_LINE_SIZE, CMDLINE_STRLEN, CMDLINE_STRLCAT);
> +
> +	if (result == true && !strncmp(test_boot_command_line, test1_result, COMMAND_LINE_SIZE)) {
> +		pr_info("test1 success.\n");
> +	} else {
> +		pr_info("test1 failed. OUTPUT BELOW:\n");
> +		pr_info("\"%s\"\n", test_boot_command_line);
> +		failed_tests++;
> +	}
> +	total_tests++;
> +
> +	/* Missing prepend */
> +	strcpy(test_boot_command_line, test2_bootloader_args);
> +	test_tmp[0] = '\0';
> +	result = __cmdline_add_builtin(test_boot_command_line, test_tmp, "", test2_append, COMMAND_LINE_SIZE, CMDLINE_STRLEN, CMDLINE_STRLCAT);
> +
> +	if (result == true && !strncmp(test_boot_command_line, test2_result, COMMAND_LINE_SIZE)) {
> +		pr_info("test2 success.\n");
> +	} else {
> +		pr_info("test2 failed. OUTPUT BELOW:\n");
> +		pr_info("\"%s\"\n", test_boot_command_line);
> +		failed_tests++;
> +	}

Can you refactor and avoid repeating those 7 lines five times ?

> +	total_tests++;
> +
> +	/* Missing append */
> +	strcpy(test_boot_command_line, test3_bootloader_args);
> +	test_tmp[0] = '\0';
> +	result = __cmdline_add_builtin(test_boot_command_line, test_tmp, test3_prepend, "", COMMAND_LINE_SIZE, CMDLINE_STRLEN, CMDLINE_STRLCAT);
> +
> +	if (result == true && !strncmp(test_boot_command_line, test3_result, COMMAND_LINE_SIZE)) {
> +		pr_info("test3 success.\n");
> +	} else {
> +		pr_info("test3 failed. OUTPUT BELOW:\n");
> +		pr_info("\"%s\"\n", test_boot_command_line);
> +		failed_tests++;
> +	}
> +	total_tests++;
> +
> +	/* Missing append and prepend */
> +	strcpy(test_boot_command_line, test4_bootloader_args);
> +	test_tmp[0] = '\0';
> +	result = __cmdline_add_builtin(test_boot_command_line, test_tmp, "", "", COMMAND_LINE_SIZE, CMDLINE_STRLEN, CMDLINE_STRLCAT);
> +
> +	if (result == true && !strncmp(test_boot_command_line, test4_result, COMMAND_LINE_SIZE)) {
> +		pr_info("test4 success.\n");
> +	} else {
> +		pr_info("test4 failed. OUTPUT BELOW:\n");
> +		pr_info("\"%s\"\n", test_boot_command_line);
> +		failed_tests++;
> +	}
> +	total_tests++;
> +
> +	/* Already full boot arguments */
> +	strcpy(test5_boot_command_line, test5_bootloader_args);
> +	test_tmp[0] = '\0';
> +	result = __cmdline_add_builtin(test5_boot_command_line, test_tmp, test5_prepend, test5_append, 512, CMDLINE_STRLEN, CMDLINE_STRLCAT);
> +
> +	if (result == false && !strncmp(test5_boot_command_line, test5_result, COMMAND_LINE_SIZE)) {
> +		pr_info("test5 success.\n");
> +	} else {
> +		pr_info("test5 failed. OUTPUT BELOW:\n");
> +		pr_info("\"%s\"\n", test5_boot_command_line);
> +		failed_tests++;
> +	}
> +	total_tests++;
> +}
> +
> +KSTM_MODULE_LOADERS(cmdline_test);
> +MODULE_AUTHOR("Daniel Walker <danielwa@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");