Re: [PATCH 2/3] powerpc: convert to generic builtin command line

From: Christophe Leroy
Date: Mon Mar 04 2019 - 10:04:11 EST



Le 04/03/2019 Ã 15:26, Christophe Leroy a ÃcritÂ:


Le 01/03/2019 Ã 20:44, Daniel Walker a ÃcritÂ:
This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
option.

Please explain more in details how each powerpc option is replaced by one of the generic options.


[maksym.kokhan@xxxxxxxxxxxxxxx: add strlcat to prom_init_check.sh
whitelist]
Cc: Daniel Walker <dwalker@xxxxxxxxxx>
Cc: Daniel Walker <danielwa@xxxxxxxxx>
Cc: xe-linux-external@xxxxxxxxx
Signed-off-by: Daniel Walker <danielwa@xxxxxxxxx>
Signed-off-by: Maksym Kokhan <maksym.kokhan@xxxxxxxxxxxxxxx>
---
 arch/powerpc/Kconfig | 23 +----------------------
 arch/powerpc/kernel/prom.c | 4 ++++
 arch/powerpc/kernel/prom_init.c | 8 ++++----
 arch/powerpc/kernel/prom_init_check.sh | 2 +-
 4 files changed, 10 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8be31261aec8..6321b2a0b87b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -172,6 +172,7 @@ config PPC
ÂÂÂÂÂ select GENERIC_STRNCPY_FROM_USER
ÂÂÂÂÂ select GENERIC_STRNLEN_USER
ÂÂÂÂÂ select GENERIC_TIME_VSYSCALL
+ÂÂÂ select GENERIC_CMDLINE
ÂÂÂÂÂ select HAVE_ARCH_AUDITSYSCALL
ÂÂÂÂÂ select HAVE_ARCH_JUMP_LABEL
ÂÂÂÂÂ select HAVE_ARCH_KGDB
@@ -777,28 +778,6 @@ config PPC_DENORMALISATION
ÂÂÂÂÂÂÂ Add support for handling denormalisation of single precision
 values. Useful for bare metal only. If unsure say Y here.
-config CMDLINE_BOOL
-ÂÂÂ bool "Default bootloader kernel arguments"
-
-config CMDLINE
-ÂÂÂ string "Initial kernel command string"
-ÂÂÂ depends on CMDLINE_BOOL
-ÂÂÂ default "console=ttyS0,9600 console=tty0 root=/dev/sda2"
-ÂÂÂ 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_FORCE
-ÂÂÂ bool "Always use the default kernel command string"
-ÂÂÂ depends on CMDLINE_BOOL
-ÂÂÂ 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.
-
 config EXTRA_TARGETS
ÂÂÂÂÂ string "Additional default image types"
ÂÂÂÂÂ help
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index fe758cedb93f..d78b1d6fe1c8 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -34,6 +34,7 @@
 #include <linux/of_fdt.h>
 #include <linux/libfdt.h>
 #include <linux/cpu.h>
+#include <linux/cmdline.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
@@ -716,6 +717,9 @@ void __init early_init_devtree(void *params)
ÂÂÂÂÂÂ */
ÂÂÂÂÂ of_scan_flat_dt(early_init_dt_scan_chosen_ppc, boot_command_line);
+ÂÂÂ /* append and prepend any arguments built into the kernel. */
+ÂÂÂ cmdline_add_builtin(boot_command_line, NULL, COMMAND_LINE_SIZE);
+

I don't think it is worth an implementation as complex as in the previous patch just for the above line.
Could easily define the temporary buffer in this file directely, then just locally do:

strlcpy(temp_buff, CONFIG_CMDLINE_PREPEND, COMMAND_LINE_SIZE);
strlcat(temp_buff, boot_command_line, COMMAND_LINE_SIZE);
strlcat(temp_buff, CONFIG_CMDLINE_APPEND, COMMAND_LINE_SIZE);
strlcpy(boot_command_line, temp_buff, COMMAND_LINE_SIZE);

And in fact, what should really be done is not the above but simply update early_init_dt_scan_chosen_ppc() in drivers/of/fdt.c as it is there that CONFIG_CMDLINE and CONFIG_CMDLINE_FORCE and CONFIG_CMDLINE_EXTEND are handled.

It looks to me that the current implementation is rather complete and eventually only missing the prepend. Why not just add prepend to the current implementation, and also move CONFIG_CMDLINE etc ... into arch/Kconfig instead of having it in each arch ?

Christophe




ÂÂÂÂÂ /* Scan memory nodes and rebuild MEMBLOCKs */
ÂÂÂÂÂ of_scan_flat_dt(early_init_dt_scan_root, NULL);
ÂÂÂÂÂ of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index f33ff4163a51..e8e9fca22470 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -30,6 +30,7 @@
 #include <linux/delay.h>
 #include <linux/initrd.h>
 #include <linux/bitops.h>
+#include <linux/cmdline.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
 #include <asm/page.h>
@@ -637,11 +638,10 @@ static void __init early_cmdline_parse(void)
ÂÂÂÂÂ p = prom_cmd_line;
ÂÂÂÂÂ if ((long)prom.chosen > 0)
ÂÂÂÂÂÂÂÂÂ l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
-#ifdef CONFIG_CMDLINE
+
ÂÂÂÂÂ if (l <= 0 || p[0] == '\0') /* dbl check */
-ÂÂÂÂÂÂÂ strlcpy(prom_cmd_line,
-ÂÂÂÂÂÂÂÂÂÂÂ CONFIG_CMDLINE, sizeof(prom_cmd_line));
-#endif /* CONFIG_CMDLINE */
+ÂÂÂÂÂÂÂ cmdline_add_builtin_section(prom_cmd_line, NULL, sizeof(prom_cmd_line), __prombss);
+

You don't need something as complex as what your generic code does for that. It could be done with the following simple line:

strlcpy(prom_cmd_line, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND, sizeof(prom_cmd_line));

ÂÂÂÂÂ prom_printf("command line: %s\n", prom_cmd_line);
 #ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/prom_init_check.sh b/arch/powerpc/kernel/prom_init_check.sh
index 667df97d2595..ab2acc8d8b5a 100644
--- a/arch/powerpc/kernel/prom_init_check.sh
+++ b/arch/powerpc/kernel/prom_init_check.sh
@@ -18,7 +18,7 @@
 WHITELIST="add_reloc_offset __bss_start __bss_stop copy_and_flush
 _end enter_prom memcpy memset reloc_offset __secondary_hold
-__secondary_hold_acknowledge __secondary_hold_spinloop __start
+__secondary_hold_acknowledge __secondary_hold_spinloop __start strlcat

The above is a big issue. In the scope of KASAN implementation, we are getting rid of generic string functions from prom_init because they are KASAN instrumented and that's far too early for prom_init. See series https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=94949 and especially patch [v9,03/11] powerpc/prom_init: don't use string functions from lib/

 strcmp strcpy strlcpy strlen strncmp strstr kstrtobool logo_linux_clut224
 reloc_got2 kernstart_addr memstart_addr linux_banner _stext
 __prom_init_toc_start __prom_init_toc_end btext_setup_display TOC."


Thanks
Christophe