Re: [PATCH 1/8] CMDLINE: add generic builtin command line
On 11/10/2023 7:08 AM, Daniel Walker wrote: > diff --git a/lib/generic_cmdline.S b/lib/generic_cmdline.S > new file mode 100644 > index ..223763f9eeb6 > --- /dev/null > +++ b/lib/generic_cmdline.S > @@ -0,0 +1,53 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#include > +#include > + > +#include > + > +__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) > + Hi Daniel, I picked these patches to test a usecase of ours. generic_cmdline.S does not escape semicolons in the CMDLINE_APPEND and CMDLINE_PREPEND strings. Take this config snippet for example: CONFIG_CMDLINE_APPEND="slub_debug=FZP,zs_handle,zspage;FZPU page_owner=on" CONFIG_CMDLINE_BOOL=y # CONFIG_CMDLINE_EXTRA is not set # CONFIG_CMDLINE_OVERRIDE is not set # CONFIG_CMDLINE_PREPEND is not set # CONFIG_TEST_CMDLINE is not set While compiling, the word FZPU is considered as a mnemonic because of the semicolon preceding it causing parsing to fail: kernel/lib/generic_cmdline.S: Assembler messages: kernel/lib/generic_cmdline.S:42: Warning: missing closing `"' kernel/lib/generic_cmdline.S:42: Error: unknown mnemonic `fzpu' -- `fzpu page_owner=on",""' Maybe Christophe's suggestion of moving this code to a C file and using inline assembly helps mitigate similar problems? Thanks, Jaskaran.
Re: [PATCH 1/8] CMDLINE: add generic builtin command line
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-exter...@cisco.com
> Signed-off-by: Ruslan Bilovol
> Signed-off-by: Daniel Walker
> ---
> 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 ..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
> +#include
> +#include
> +
> +#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,
Re: [PATCH 1/8] CMDLINE: add generic builtin command line
Hi Daniel,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.6]
[cannot apply to arm64/for-next/core efi/next tip/x86/core robh/for-next
linus/master next-20231110]
[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#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Daniel-Walker/CMDLINE-add-generic-builtin-command-line/20231110-094423
base: v6.6
patch link:
https://lore.kernel.org/r/20231110013817.2378507-2-danielwa%40cisco.com
patch subject: [PATCH 1/8] CMDLINE: add generic builtin command line
config: sparc-allyesconfig
(https://download.01.org/0day-ci/archive/20231110/202311102331.gllfai9t-...@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20231110/202311102331.gllfai9t-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202311102331.gllfai9t-...@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:15,
from include/linux/interrupt.h:6,
from arch/sparc/include/asm/setup.h:8,
from lib/generic_cmdline.S:5:
>> include/linux/align.h:8: warning: "ALIGN" redefined
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
|
In file included from include/linux/export.h:6,
from lib/generic_cmdline.S:2:
include/linux/linkage.h:103: note: this is the location of the previous
definition
103 | #define ALIGN __ALIGN
|
In file included from include/linux/kcsan-checks.h:13,
from include/linux/instrumented.h:12,
from include/linux/atomic/atomic-instrumented.h:17,
from include/linux/atomic.h:82,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22:
>> include/linux/compiler_attributes.h:55: warning: "__always_inline" redefined
55 | #define __always_inline inline
__attribute__((__always_inline__))
|
In file included from include/linux/stddef.h:5,
from include/linux/kernel.h:18:
include/uapi/linux/stddef.h:8: note: this is the location of the previous
definition
8 | #define __always_inline inline
|
>> include/linux/compiler_attributes.h:91:5: warning: "__has_attribute" is not
>> defined, evaluates to 0 [-Wundef]
91 | #if __has_attribute(__copy__)
| ^~~
>> include/linux/compiler_attributes.h:91:20: error: missing binary operator
>> before token "("
91 | #if __has_attribute(__copy__)
|^
include/linux/compiler_attributes.h:104:5: warning: "__has_attribute" is not
defined, evaluates to 0 [-Wundef]
104 | #if __has_attribute(__counted_by__)
| ^~~
include/linux/compiler_attributes.h:104:20: error: missing binary operator
before token "("
104 | #if __has_attribute(__counted_by__)
|^
>> include/linux/compiler_attributes.h:107: warning: "__counted_by" redefined
107 | # define __counted_by(member)
|
include/uapi/linux/stddef.h:55: note: this is the location of the previous
definition
55 | #define __counted_by(m)
|
include/linux/compiler_attributes.h:116:5: warning: "__has_attribute" is not
defined, evaluates to 0 [-Wundef]
116 | #if __has_attribute(__diagnose_as_builtin__)
| ^~~
include/linux/compiler_attributes.h:116:20: error: missing binary operator
before token "("
116 | #if __has_attribute(__diagnose_as_builtin__)
|^
include/linux/compiler_attributes.h:139:5: warning: "__has_attribute" is not
defined, evaluates to 0 [-Wundef]
139 | #if __has_attribute(__designated_init__)
| ^~~
include/linux/compiler_attributes.h:139:20: error: missing binary operator
before token "("
139 | #if __has_attribute(__designated_init__)
|^
include/linux/compiler_attributes.h:150:5: warning: "__has_attribute" is not
defined, evaluates to 0 [-Wundef]
150 | #if __has_attribute(__error__)
| ^~~
include/linux/compiler_attributes.h:150:20: error: missing binary operator
before token "("
150 | #if __has_attribute(__error__)
|^
Re: [PATCH 1/8] CMDLINE: add generic builtin command line
Le 30/03/2021 à 19:56, 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.
Can you explain in the commit what is the need ? Nobody mind "who" needs it I think, but "what" is
needed would be valuable to know.
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.
This unified implementation offers the same functionality needed by
Cisco on all platform which use it.
Cisco cisco cisco ... Can we avoid mentionning companies like this ? I can't see patches mentioning
google or IBM or other companies to that extend.
Cc: xe-linux-exter...@cisco.com
Signed-off-by: Ruslan Bilovol
Signed-off-by: Daniel Walker
---
include/linux/cmdline.h | 98 +
init/Kconfig| 72 ++
2 files changed, 170 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 ..439c4585feba
--- /dev/null
+++ b/include/linux/cmdline.h
@@ -0,0 +1,98 @@
+/* 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.
+ */
+
+#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
+
+#ifndef CONFIG_CMDLINE_OVERRIDE
+#define GENERIC_CMDLINE_NEED_STRLCAT
Does it matter ?
Only powerpc needs that really. And prom_strlcat() is there anyway, so why not just use it when
needed and rely on GCC to optimise it out when possible ?
+#define CMDLINE_PREPEND CONFIG_CMDLINE_PREPEND
+#define CMDLINE_APPEND CONFIG_CMDLINE_APPEND
What are those defines used for ?
+
+/*
+ * 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
+ * @src: The starting string or NULL if there isn't one.
+ * @tmp: temporary space used for prepending
+ * @length: the maximum length of the strings above.
+ * @cmdline_strlcpy: point to a compatible strlcpy
+ * @cmdline_strlcat: point to a compatible strlcat
+ */
+static inline void
+__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long
length,
+ size_t (*cmdline_strlcpy)(char *dest, const char *src, size_t
size),
+ size_t (*cmdline_strlcat)(char *dest, const char *src, size_t
count))
I still can see the advantage of passing strlcpy and strlcat as functions to
the function.
Can we instead use macros defined by default that can be overriden by powerpc ?
Something like
#ifndef cmdline_strlcat
#define cmdline_strlcat strlcat
#define cmdline_strlcpy strlcpy
#endif
+{
+ if (src != dest && src != NULL) {
+ cmdline_strlcpy(dest, " ", length);
+ cmdline_strlcat(dest, src, length);
+ }
+
+ if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
+ cmdline_strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
+
+ if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
+ cmdline_strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
+ cmdline_strlcat(tmp, dest, length);
+ cmdline_strlcpy(dest, tmp, length);
+ }
+}
+
+#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy,
cmdline_strlcat) \
+{
\
+ if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
\
+ static label char cmdline_tmp_space[length];
\
+ __cmdline_add_builtin(dest, src, cmdline_tmp_space, length,
cmdline_strlcpy, cmdline_strlcat); \
+ } else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) {
\
+ __cmdline_add_builtin(dest, src, NULL, length, cmdline_strlcpy,
cmdline_strlcat); \
+ }
\
+}
I still don't like passing section names to a macro that way, just for powerpc.
That tmp space is only needed when source and destination are identical, and it is easy to ensure
powerpc doesn't need that.
