On Sat, 29 Jul 2023 09:16:56 -0700 "Paul E. McKenney" <paulmck@xxxxxxxxxx> wrote: > On Sat, Jul 29, 2023 at 11:29:29PM +0900, Masami Hiramatsu wrote: > > Hi Paul, > > > > On Thu, 27 Jul 2023 20:37:00 -0700 > > "Paul E. McKenney" <paulmck@xxxxxxxxxx> wrote: > > > > > In kernels built with CONFIG_BOOT_CONFIG_FORCE=y, /proc/cmdline will > > > show all kernel boot parameters, both those supplied by the boot loader > > > and those embedded in the kernel image. This works well for those who > > > just want to see all of the kernel boot parameters, but is not helpful to > > > those who need to see only those parameters supplied by the boot loader. > > > This is especially important when these parameters are presented to the > > > boot loader by automation that might gather them from diverse sources. > > > > > > Therefore, provide a /proc/cmdline_load file that shows only those kernel > > > boot parameters supplied by the boot loader. > > > > If I understand correctly, /proc/cmdline_load is something like > > /proc/cmdline_load - `/proc/bootconfig | grep ^kernel\\.`. ^^^ /proc/cmdline - `/proc/bootconfig | grep ^kernel\\.` > > Yes, very much something like that. > > For one use case, suppose you have a kernel that gets some boot parameters > from the boot loader and some from bootconfig. If you want to kexec() > into a new kernel, you must tell kexec() what the kernel boot parameters > are. However, you must *not* tell kexec() about any of the current > kernel's parameters that came from bootconfig, because those should > instead be supplied by the new kernel being kexec()ed into. > > So you must pass in only those parameters that came from the boot loader, > hence my proposed /proc/cmdline_load. Ah, I got it. Indeed, for kexec, we need to drop the options from the bootconfig. > > > BTW, what about CONFIG_CMDLINE? We already have that Kconfig and it is also > > merged with the command line specified by boot loader. Should we also > > expose that? (when CONFIG_CMDLINE_OVERRIDE=y, we don't need it because > > cmdline is always overridden by the CONFIG_CMDLINE) Unfortunatelly, this > > option is implemented in each arch init, so we have to change all of them... > > The use case is embedded systems, right? I have no idea whether they > have a use case requiring this. Do those sorts of embedded systems > use kexec()? (I don't know of any that do, but then again, I haven't > been looking.) Not sure, I guess it is possible to use kexec() for kdump or warm reboot, but it should be rare and we can expand this if someone need it. > > This arch init is in setup_arch(), correct? If so, one option is to > make start_kernel() or something that it invokes make a copy of the > command line just before invoking setup_arch(). Full disclosure: I > have not yet looked at all the ins and outs of CONFIG_CMDLINE, so this > suggestion should be viewed with appropriate skepticism. Yeah, maybe it is the best way to do. Anyway, I understand the reason why we need this interface. Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> Thank you! > > > Thank you, > > > > > > > > Why put this in /proc? Because it is quite similar to /proc/cmdline, so > > > it makes sense to put it in the same place that /proc/cmdline is located. > > > > > > [ sfr: Apply kernel test robot feedback. ] > > > > > > Co-developed-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> > > > Signed-off-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> > > > Co-developed-by: Arnd Bergmann <arnd@xxxxxxxxxx> > > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxxxx> > > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx> > > > Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> > > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > > Cc: Alexey Dobriyan <adobriyan@xxxxxxxxx> > > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx> > > > Cc: <linux-fsdevel@xxxxxxxxxxxxxxx> > > > --- > > > fs/proc/cmdline.c | 13 +++++++++++++ > > > include/linux/init.h | 3 ++- > > > init/main.c | 2 +- > > > 3 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c > > > index a6f76121955f..1d0ef9d2949d 100644 > > > --- a/fs/proc/cmdline.c > > > +++ b/fs/proc/cmdline.c > > > @@ -3,6 +3,7 @@ > > > #include <linux/init.h> > > > #include <linux/proc_fs.h> > > > #include <linux/seq_file.h> > > > +#include <asm/setup.h> > > > #include "internal.h" > > > > > > static int cmdline_proc_show(struct seq_file *m, void *v) > > > @@ -12,6 +13,13 @@ static int cmdline_proc_show(struct seq_file *m, void *v) > > > return 0; > > > } > > > > > > +static int cmdline_load_proc_show(struct seq_file *m, void *v) > > > +{ > > > + seq_puts(m, boot_command_line); > > > + seq_putc(m, '\n'); > > > + return 0; > > > +} > > > + > > > static int __init proc_cmdline_init(void) > > > { > > > struct proc_dir_entry *pde; > > > @@ -19,6 +27,11 @@ static int __init proc_cmdline_init(void) > > > pde = proc_create_single("cmdline", 0, NULL, cmdline_proc_show); > > > pde_make_permanent(pde); > > > pde->size = saved_command_line_len + 1; > > > + if (IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)) { > > > + pde = proc_create_single("cmdline_load", 0, NULL, cmdline_load_proc_show); > > > + pde_make_permanent(pde); > > > + pde->size = strnlen(boot_command_line, COMMAND_LINE_SIZE) + 1; > > > + } > > > return 0; > > > } > > > fs_initcall(proc_cmdline_init); > > > diff --git a/include/linux/init.h b/include/linux/init.h > > > index 266c3e1640d4..29e75bbe7984 100644 > > > --- a/include/linux/init.h > > > +++ b/include/linux/init.h > > > @@ -112,6 +112,7 @@ > > > #define __REFCONST .section ".ref.rodata", "a" > > > > > > #ifndef __ASSEMBLY__ > > > + > > > /* > > > * Used for initialization calls.. > > > */ > > > @@ -143,7 +144,7 @@ struct file_system_type; > > > > > > /* Defined in init/main.c */ > > > extern int do_one_initcall(initcall_t fn); > > > -extern char __initdata boot_command_line[]; > > > +extern char boot_command_line[]; > > > > FYI, boot_command_line[] is mixture of built-in cmdline string with > > bootloader cmdline string. > > So if we also need to separate out the CONFIG_CMDLINE arguments, then > /proc/cmdline_load will need to come from some string saved off before > the CONFIG_CMDLINE processing, correct? I would expect that to be a > separate patch series, but if it is needed, I would be happy to look > into setting it up, as long as I am in the area. > > My tests indicate that boot_command_line[] doesn't contain any bootconfig > (and opposed to CONFIG_CMDLINE) arguments, but I could easily have missed > some other corner-case configuration. > > And thank you for looking this over! > > Thanx, Paul > > > > extern char *saved_command_line; > > > extern unsigned int saved_command_line_len; > > > extern unsigned int reset_devices; > > > diff --git a/init/main.c b/init/main.c > > > index ad920fac325c..2121685c479a 100644 > > > --- a/init/main.c > > > +++ b/init/main.c > > > @@ -135,7 +135,7 @@ EXPORT_SYMBOL(system_state); > > > void (*__initdata late_time_init)(void); > > > > > > /* Untouched command line saved by arch-specific code. */ > > > -char __initdata boot_command_line[COMMAND_LINE_SIZE]; > > > +char boot_command_line[COMMAND_LINE_SIZE] __ro_after_init; > > > /* Untouched saved command line (eg. for /proc) */ > > > char *saved_command_line __ro_after_init; > > > unsigned int saved_command_line_len __ro_after_init; > > > -- > > > 2.40.1 > > > > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> -- Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>