Message ID | cover.1617375802.git.christophe.leroy@csgroup.eu |
---|---|
Headers | show |
Series | Implement GENERIC_CMDLINE | expand |
On Fri, Apr 02, 2021 at 03:18:01PM +0000, Christophe Leroy wrote: > The purpose of this series is to improve and enhance the > handling of kernel boot arguments. > > Current situation is that most if not all architectures are using > similar options to do some manupulation on command line arguments: > - Prepend built-in arguments in front of bootloader provided arguments > - Append built-in arguments after bootloader provided arguments > - Replace bootloader provided arguments by built-in arguments > - Use built-in arguments when none is provided by bootloader. > > On some architectures, all the options are possible. On other ones, > only a subset are available. > > The purpose of this series is to refactor and enhance the > handling of kernel boot arguments so that every architecture can > benefit from all possibilities. > > It is first focussed on powerpc but also extends the capability > for other arches. > > The work has been focussed on minimising the churn in architectures > by keeping the most commonly used namings. > > Main changes in V4: > - Included patch from Daniel to replace powerpc's strcpy() by strlcpy() > - Using strlcpy() instead of zeroing first char + strlcat() (idea taken frm Daniel's series) > - Reworked the convertion of EFI which was wrong in V3 > - Added "too long" command line handling > - Changed cmdline macro into a function > - Done a few fixes in arch (NIOS2, SH, ARM) > - Taken comments into account (see individual responses for details) > - Tested on powerpc, build tested on ARM64, X86_64. > Why submit your changes ? My changes have been around for almost 10 years, and are more widely used. Your changes are very new and unstable, but don't really solve the needs of people using my series. I've tried to work with you and I take comments from you, but yet you insist to submit your own series. I would suggest this isn't going to go anyplace unless we work together. I can't really support your changes because, honestly, your changes are really ugly and they just look more and more like my changes with every passing iteration .. As the maturity of your changes continue they will just become my change set. I've been thru every iteration of these changes, and I see those attempts in your changes. Everything different in your changes I've tried, and found not to be useful, then it falls away in later iterations. When you give me comments on something which I haven't tried I typically incorporate it. Daniel
On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote: > -config CMDLINE_BOOL > - bool "Built-in kernel command line" > - help > - For most systems, it is firmware or second stage bootloader that > - by default specifies the kernel command line options. However, > - it might be necessary or advantageous to either override the > - default kernel command line or add a few extra options to it. > - For such cases, this option allows you to hardcode your own > - command line options directly into the kernel. For that, you > - should choose 'Y' here, and fill in the extra boot arguments > - in CONFIG_CMDLINE. > - > - The built-in options will be concatenated to the default command > - line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default > - command line will be ignored and replaced by the built-in string. > - > - Most MIPS systems will normally expect 'N' here and rely upon > - the command line from the firmware or the second-stage bootloader. > - See how you complained that I have CMDLINE_BOOL in my changed, and you think it shouldn't exist. Yet here mips has it, and you just deleted it with no feature parity in your changes for this. In my changes I tried to maintain as much feature parity as I could with the architectures. I did the same huge conversion a long time ago you've done here to be sure all platforms have the features needed. Daniel
On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote: > On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote: > > -config CMDLINE_BOOL > > - bool "Built-in kernel command line" > > - help > > - For most systems, it is firmware or second stage bootloader that > > - by default specifies the kernel command line options. However, > > - it might be necessary or advantageous to either override the > > - default kernel command line or add a few extra options to it. > > - For such cases, this option allows you to hardcode your own > > - command line options directly into the kernel. For that, you > > - should choose 'Y' here, and fill in the extra boot arguments > > - in CONFIG_CMDLINE. > > - > > - The built-in options will be concatenated to the default command > > - line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default > > - command line will be ignored and replaced by the built-in string. > > - > > - Most MIPS systems will normally expect 'N' here and rely upon > > - the command line from the firmware or the second-stage bootloader. > > - > > > See how you complained that I have CMDLINE_BOOL in my changed, and you think it > shouldn't exist. > > Yet here mips has it, and you just deleted it with no feature parity in your > changes for this. AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You seem to need it just because you have CMDLINE_PREPEND and CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL is not a feature, but an implementation detail. Rob
On Fri, Apr 02, 2021 at 03:18:20PM +0000, Christophe Leroy wrote: > This converts the architecture to GENERIC_CMDLINE. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/x86/Kconfig | 45 ++--------------------------------------- > arch/x86/kernel/setup.c | 17 ++-------------- > 2 files changed, 4 insertions(+), 58 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index a20684d56b4b..66b384228ca3 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -104,6 +104,7 @@ config X86 > select ARCH_USE_QUEUED_SPINLOCKS > select ARCH_USE_SYM_ANNOTATIONS > select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH > + select ARCH_WANT_CMDLINE_PREPEND_BY_DEFAULT Seems to be non-existent kconfig option. > select ARCH_WANT_DEFAULT_BPF_JIT if X86_64 > select ARCH_WANTS_DYNAMIC_TASK_STRUCT > select ARCH_WANT_HUGE_PMD_SHARE > @@ -118,6 +119,7 @@ config X86 > select EDAC_SUPPORT > select GENERIC_CLOCKEVENTS_BROADCAST if X86_64 || (X86_32 && X86_LOCAL_APIC) > select GENERIC_CLOCKEVENTS_MIN_ADJUST > + select GENERIC_CMDLINE > select GENERIC_CMOS_UPDATE > select GENERIC_CPU_AUTOPROBE > select GENERIC_CPU_VULNERABILITIES > @@ -2358,49 +2360,6 @@ choice > > endchoice > > -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 > - boot arguments in CONFIG_CMDLINE. > - > - Systems with fully functional boot loaders (i.e. non-embedded) > - should leave this option set to 'N'. > - > -config CMDLINE > - string "Built-in kernel command string" > - 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, it is appended to this string to > - form the full kernel command line, when the system boots. > - > - However, you can use the CONFIG_CMDLINE_FORCE 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_FORCE > - bool "Built-in command line overrides boot loader arguments" > - depends on CMDLINE_BOOL && CMDLINE != "" > - help > - Set this option to 'Y' to have the kernel ignore the boot loader > - command line, and use ONLY the built-in command line. > - > - This is used to work around broken boot loaders. This should > - be set to 'N' under normal conditions. > - > config MODIFY_LDT_SYSCALL > bool "Enable the LDT (local descriptor table)" if EXPERT > default y > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 6f2de58eeb54..3f274b02e51c 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -5,6 +5,7 @@ > * This file contains the setup_arch() code, which handles the architecture-dependent > * parts of early kernel initialization. > */ > +#include <linux/cmdline.h> > #include <linux/console.h> > #include <linux/crash_dump.h> > #include <linux/dma-map-ops.h> > @@ -161,9 +162,6 @@ unsigned long saved_video_mode; > #define RAMDISK_LOAD_FLAG 0x4000 > > static char __initdata command_line[COMMAND_LINE_SIZE]; > -#ifdef CONFIG_CMDLINE_BOOL > -static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE; > -#endif > > #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE) > struct edd edd; > @@ -883,18 +881,7 @@ void __init setup_arch(char **cmdline_p) > bss_resource.start = __pa_symbol(__bss_start); > bss_resource.end = __pa_symbol(__bss_stop)-1; > > -#ifdef CONFIG_CMDLINE_BOOL > -#ifdef CONFIG_CMDLINE_FORCE > - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > -#else > - if (builtin_cmdline[0]) { > - /* append boot loader cmdline to builtin */ > - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); > - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); > - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > - } > -#endif > -#endif > + cmdline_build(boot_command_line, boot_command_line); > > strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE); > *cmdline_p = command_line; Once this is all done, I wonder if we can get rid of the strlcpy and perhaps also cmdline_p. Rob
On Thu, Apr 08, 2021 at 02:04:08PM -0500, Rob Herring wrote: > On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote: > > On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote: > > > -config CMDLINE_BOOL > > > - bool "Built-in kernel command line" > > > - help > > > - For most systems, it is firmware or second stage bootloader that > > > - by default specifies the kernel command line options. However, > > > - it might be necessary or advantageous to either override the > > > - default kernel command line or add a few extra options to it. > > > - For such cases, this option allows you to hardcode your own > > > - command line options directly into the kernel. For that, you > > > - should choose 'Y' here, and fill in the extra boot arguments > > > - in CONFIG_CMDLINE. > > > - > > > - The built-in options will be concatenated to the default command > > > - line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default > > > - command line will be ignored and replaced by the built-in string. > > > - > > > - Most MIPS systems will normally expect 'N' here and rely upon > > > - the command line from the firmware or the second-stage bootloader. > > > - > > > > > > See how you complained that I have CMDLINE_BOOL in my changed, and you think it > > shouldn't exist. > > > > Yet here mips has it, and you just deleted it with no feature parity in your > > changes for this. > > AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You > seem to need it just because you have CMDLINE_PREPEND and > CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL > is not a feature, but an implementation detail. Not true. It makes it easier to turn it all off inside the Kconfig , so it's for usability and multiple architecture have it even with just CMDLINE as I was commenting here. Daniel
Le 08/04/2021 à 21:41, Rob Herring a écrit : > On Fri, Apr 02, 2021 at 03:18:20PM +0000, Christophe Leroy wrote: >> This converts the architecture to GENERIC_CMDLINE. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/x86/Kconfig | 45 ++--------------------------------------- >> arch/x86/kernel/setup.c | 17 ++-------------- >> 2 files changed, 4 insertions(+), 58 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index a20684d56b4b..66b384228ca3 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -104,6 +104,7 @@ config X86 >> select ARCH_USE_QUEUED_SPINLOCKS >> select ARCH_USE_SYM_ANNOTATIONS >> select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH >> + select ARCH_WANT_CMDLINE_PREPEND_BY_DEFAULT > > Seems to be non-existent kconfig option. Oops. Added in v5. >> @@ -883,18 +881,7 @@ void __init setup_arch(char **cmdline_p) >> bss_resource.start = __pa_symbol(__bss_start); >> bss_resource.end = __pa_symbol(__bss_stop)-1; >> >> -#ifdef CONFIG_CMDLINE_BOOL >> -#ifdef CONFIG_CMDLINE_FORCE >> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); >> -#else >> - if (builtin_cmdline[0]) { >> - /* append boot loader cmdline to builtin */ >> - strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE); >> - strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE); >> - strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); >> - } >> -#endif >> -#endif >> + cmdline_build(boot_command_line, boot_command_line); >> >> strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE); >> *cmdline_p = command_line; > > Once this is all done, I wonder if we can get rid of the strlcpy and > perhaps also cmdline_p. > It seems rather complicated, in init/main.c you have heavy manipulations of command lines which seems to be done in setup_command_line() which seems to add stuff in front of command lines, at the end we end up with several command lines: /* Untouched saved command line (eg. for /proc) */ char *saved_command_line; /* Command line for parameter parsing */ static char *static_command_line; /* Untouched extra command line */ static char *extra_command_line; Some of them come from the cmdline_p which others are from boot_command_line. I think a cleanup on all that stuff would be worth it as a further step.
Le 09/04/2021 à 03:23, Daniel Walker a écrit : > On Thu, Apr 08, 2021 at 02:04:08PM -0500, Rob Herring wrote: >> On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote: >>> On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote: >>>> -config CMDLINE_BOOL >>>> - bool "Built-in kernel command line" >>>> - help >>>> - For most systems, it is firmware or second stage bootloader that >>>> - by default specifies the kernel command line options. However, >>>> - it might be necessary or advantageous to either override the >>>> - default kernel command line or add a few extra options to it. >>>> - For such cases, this option allows you to hardcode your own >>>> - command line options directly into the kernel. For that, you >>>> - should choose 'Y' here, and fill in the extra boot arguments >>>> - in CONFIG_CMDLINE. >>>> - >>>> - The built-in options will be concatenated to the default command >>>> - line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default >>>> - command line will be ignored and replaced by the built-in string. >>>> - >>>> - Most MIPS systems will normally expect 'N' here and rely upon >>>> - the command line from the firmware or the second-stage bootloader. >>>> - >>> >>> >>> See how you complained that I have CMDLINE_BOOL in my changed, and you think it >>> shouldn't exist. >>> >>> Yet here mips has it, and you just deleted it with no feature parity in your >>> changes for this. >> >> AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You >> seem to need it just because you have CMDLINE_PREPEND and >> CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL >> is not a feature, but an implementation detail. > > Not true. > > It makes it easier to turn it all off inside the Kconfig , so it's for usability > and multiple architecture have it even with just CMDLINE as I was commenting > here. > Among the 13 architectures having CONFIG_CMDLINE, todayb only 6 have a CONFIG_CMDLINE_BOOL in addition: arch/arm/Kconfig:config CMDLINE arch/arm64/Kconfig:config CMDLINE arch/hexagon/Kconfig:config CMDLINE arch/microblaze/Kconfig:config CMDLINE arch/mips/Kconfig.debug:config CMDLINE arch/nios2/Kconfig:config CMDLINE arch/openrisc/Kconfig:config CMDLINE arch/powerpc/Kconfig:config CMDLINE arch/riscv/Kconfig:config CMDLINE arch/sh/Kconfig:config CMDLINE arch/sparc/Kconfig:config CMDLINE arch/x86/Kconfig:config CMDLINE arch/xtensa/Kconfig:config CMDLINE arch/microblaze/Kconfig:config CMDLINE_BOOL arch/mips/Kconfig.debug:config CMDLINE_BOOL arch/nios2/Kconfig:config CMDLINE_BOOL arch/sparc/Kconfig:config CMDLINE_BOOL arch/x86/Kconfig:config CMDLINE_BOOL arch/xtensa/Kconfig:config CMDLINE_BOOL In the begining I hesitated about the CMDLINE_BOOL, at the end I decided to go the same way as what is done today in the kernel for initramfs with CONFIG_INITRAMFS_SOURCE. The problem I see within adding CONFIG_CMDLINE_BOOL for every architecture which don't have it today is that when doing a "make oldconfig" on their custom configs, thousands of users will loose their CMDLINE without notice. When we do the other way round, removing CONFIG_CMDLINE_BOOL on the 6 architectures that have it today will have no impact on existing config. Also, in order to avoid tons of #ifdefs in the code as mandated by Kernel Codying Style §21, we have to have CONFIG_CMDLINE defined at all time, so at the end CONFIG_CMDLINE_BOOL is really redundant with an empty CONFIG_CMDLINE. Unlike you, the approach I took for my series is to minimise the impact on existing implementation and existing configurations as much as possible. I know you have a different approach where you break every existing config anyway. https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation Christophe