| Message ID | cover.1614705851.git.christophe.leroy@csgroup.eu |
|---|---|
| Headers | show |
| Series | Improve boot command line handling | expand |
+Will D On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote: > > On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote: > > The purpose of this series is to improve and enhance the > > handling of kernel boot arguments. > > > > It is first focussed on powerpc but also extends the capability > > for other arches. > > > > This is based on suggestion from Daniel Walker <danielwa@cisco.com> > > > > > I don't see a point in your changes at this time. My changes are much more > mature, and you changes don't really make improvements. Not really a helpful comment. What we merge here will be from whomever is persistent and timely in their efforts. But please, work together on a common solution. This one meets my requirements of moving the kconfig and code out of the arches, supports prepend/append, and is up to date. Rob
On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: > This code provides architectures with a way to build command line > based on what is built in the kernel and what is handed over by the > bootloader, based on selected compile-time options. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 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 000000000000..ae3610bb0ee2 > --- /dev/null > +++ b/include/linux/cmdline.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_CMDLINE_H > +#define _LINUX_CMDLINE_H > + > +static __always_inline size_t cmdline_strlen(const char *s) > +{ > + const char *sc; > + > + for (sc = s; *sc != '\0'; ++sc) > + ; /* nothing */ > + return sc - s; > +} > + > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) > +{ > + size_t dsize = cmdline_strlen(dest); > + size_t len = cmdline_strlen(src); > + size_t res = dsize + len; > + > + /* This would be a bug */ > + if (dsize >= count) > + return count; > + > + dest += dsize; > + count -= dsize; > + if (len >= count) > + len = count - 1; > + memcpy(dest, src, len); > + dest[len] = 0; > + return res; > +} Why are these needed instead of using strlen and strlcat directly? > +/* > + * This function will append 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. Must not equal dest. > + * @length: the length of dest buffer. > + */ > +static __always_inline void cmdline_build(char *dest, const char *src, size_t length) > +{ > + if (length <= 0) > + return; > + > + dest[0] = 0; > + > +#ifdef CONFIG_CMDLINE > + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) { > + cmdline_strlcat(dest, CONFIG_CMDLINE, length); > + return; > + } > +#endif CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't, CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef? > + if (dest != src) > + cmdline_strlcat(dest, src, length); > +#ifdef CONFIG_CMDLINE > + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1) > + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length); > +#endif Likewise, but also I'm not sure why the sizeof() is required. Will
Le 03/03/2021 à 18:28, Will Deacon a écrit : > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: >> This code provides architectures with a way to build command line >> based on what is built in the kernel and what is handed over by the >> bootloader, based on selected compile-time options. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 62 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 000000000000..ae3610bb0ee2 >> --- /dev/null >> +++ b/include/linux/cmdline.h >> @@ -0,0 +1,62 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef _LINUX_CMDLINE_H >> +#define _LINUX_CMDLINE_H >> + >> +static __always_inline size_t cmdline_strlen(const char *s) >> +{ >> + const char *sc; >> + >> + for (sc = s; *sc != '\0'; ++sc) >> + ; /* nothing */ >> + return sc - s; >> +} >> + >> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) >> +{ >> + size_t dsize = cmdline_strlen(dest); >> + size_t len = cmdline_strlen(src); >> + size_t res = dsize + len; >> + >> + /* This would be a bug */ >> + if (dsize >= count) >> + return count; >> + >> + dest += dsize; >> + count -= dsize; >> + if (len >= count) >> + len = count - 1; >> + memcpy(dest, src, len); >> + dest[len] = 0; >> + return res; >> +} > > Why are these needed instead of using strlen and strlcat directly? Because on powerpc (at least), it will be used in prom_init, it is very early in the boot and KASAN shadow memory is not set up yet so calling generic string functions would crash the board. > >> +/* >> + * This function will append 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. Must not equal dest. >> + * @length: the length of dest buffer. >> + */ >> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length) >> +{ >> + if (length <= 0) >> + return; >> + >> + dest[0] = 0; >> + >> +#ifdef CONFIG_CMDLINE >> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) { >> + cmdline_strlcat(dest, CONFIG_CMDLINE, length); >> + return; >> + } >> +#endif > > CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't, > CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef? Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it is feasible. I can change that now. > >> + if (dest != src) >> + cmdline_strlcat(dest, src, length); >> +#ifdef CONFIG_CMDLINE >> + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1) >> + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length); >> +#endif > > Likewise, but also I'm not sure why the sizeof() is required. It is to avoid adding a white space at the end of the command line when CONFIG_CMDLINE is empty. But maybe it doesn't matter ? Christophe
On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote: > +Will D > > On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote: > > > > On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote: > > > The purpose of this series is to improve and enhance the > > > handling of kernel boot arguments. > > > > > > It is first focussed on powerpc but also extends the capability > > > for other arches. > > > > > > This is based on suggestion from Daniel Walker <danielwa@cisco.com> > > > > > > > > > I don't see a point in your changes at this time. My changes are much more > > mature, and you changes don't really make improvements. > > Not really a helpful comment. What we merge here will be from whomever > is persistent and timely in their efforts. But please, work together > on a common solution. > > This one meets my requirements of moving the kconfig and code out of > the arches, supports prepend/append, and is up to date. Maintainers are capable of merging whatever they want to merge. However, I wouldn't make hasty choices. The changes I've been submitting have been deployed on millions of router instances and are more feature rich. I believe I worked with you on this change, or something like it, https://lkml.org/lkml/2019/3/19/970 I don't think Christophe has even addressed this. I've converted many architectures, and Cisco uses my changes on at least 4 different architecture. With products deployed and tested. I will resubmit my changes as soon as I can. Daniel
On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: > This code provides architectures with a way to build command line > based on what is built in the kernel and what is handed over by the > bootloader, based on selected compile-time options. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 include/linux/cmdline.h Sorry, spotted a couple of other things... > +/* > + * This function will append 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. Must not equal dest. > + * @length: the length of dest buffer. > + */ > +static __always_inline void cmdline_build(char *dest, const char *src, size_t length) > +{ > + if (length <= 0) > + return; length is unsigned > + > + dest[0] = 0; > + > +#ifdef CONFIG_CMDLINE > + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) { > + cmdline_strlcat(dest, CONFIG_CMDLINE, length); > + return; > + } > +#endif > + if (dest != src) The kernel-doc says that @src "Must not equal dest". Will
On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote: > Le 03/03/2021 à 18:28, Will Deacon a écrit : > > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: > > > This code provides architectures with a way to build command line > > > based on what is built in the kernel and what is handed over by the > > > bootloader, based on selected compile-time options. > > > > > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > > --- > > > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 62 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 000000000000..ae3610bb0ee2 > > > --- /dev/null > > > +++ b/include/linux/cmdline.h > > > @@ -0,0 +1,62 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +#ifndef _LINUX_CMDLINE_H > > > +#define _LINUX_CMDLINE_H > > > + > > > +static __always_inline size_t cmdline_strlen(const char *s) > > > +{ > > > + const char *sc; > > > + > > > + for (sc = s; *sc != '\0'; ++sc) > > > + ; /* nothing */ > > > + return sc - s; > > > +} > > > + > > > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) > > > +{ > > > + size_t dsize = cmdline_strlen(dest); > > > + size_t len = cmdline_strlen(src); > > > + size_t res = dsize + len; > > > + > > > + /* This would be a bug */ > > > + if (dsize >= count) > > > + return count; > > > + > > > + dest += dsize; > > > + count -= dsize; > > > + if (len >= count) > > > + len = count - 1; > > > + memcpy(dest, src, len); > > > + dest[len] = 0; > > > + return res; > > > +} > > > > Why are these needed instead of using strlen and strlcat directly? > > Because on powerpc (at least), it will be used in prom_init, it is very > early in the boot and KASAN shadow memory is not set up yet so calling > generic string functions would crash the board. Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can you not do something similar? Failing that, I think it would be better to offer the option for an arch to implement cmdline_*, but have then point to the normal library routines by default. > > > +/* > > > + * This function will append 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. Must not equal dest. > > > + * @length: the length of dest buffer. > > > + */ > > > +static __always_inline void cmdline_build(char *dest, const char *src, size_t length) > > > +{ > > > + if (length <= 0) > > > + return; > > > + > > > + dest[0] = 0; > > > + > > > +#ifdef CONFIG_CMDLINE > > > + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) { > > > + cmdline_strlcat(dest, CONFIG_CMDLINE, length); > > > + return; > > > + } > > > +#endif > > > > CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't, > > CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef? > > Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it > is feasible. I can change that now. > > > > > > + if (dest != src) > > > + cmdline_strlcat(dest, src, length); > > > +#ifdef CONFIG_CMDLINE > > > + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1) > > > + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length); > > > +#endif > > > > Likewise, but also I'm not sure why the sizeof() is required. > > It is to avoid adding a white space at the end of the command line when > CONFIG_CMDLINE is empty. But maybe it doesn't matter ? If CONFIG_CMDLINE is empty, I don't think you can select CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters). Will
Le 03/03/2021 à 18:46, Will Deacon a écrit : > On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote: >> Le 03/03/2021 à 18:28, Will Deacon a écrit : >>> On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: >>>> This code provides architectures with a way to build command line >>>> based on what is built in the kernel and what is handed over by the >>>> bootloader, based on selected compile-time options. >>>> >>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>>> --- >>>> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 62 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 000000000000..ae3610bb0ee2 >>>> --- /dev/null >>>> +++ b/include/linux/cmdline.h >>>> @@ -0,0 +1,62 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +#ifndef _LINUX_CMDLINE_H >>>> +#define _LINUX_CMDLINE_H >>>> + >>>> +static __always_inline size_t cmdline_strlen(const char *s) >>>> +{ >>>> + const char *sc; >>>> + >>>> + for (sc = s; *sc != '\0'; ++sc) >>>> + ; /* nothing */ >>>> + return sc - s; >>>> +} >>>> + >>>> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) >>>> +{ >>>> + size_t dsize = cmdline_strlen(dest); >>>> + size_t len = cmdline_strlen(src); >>>> + size_t res = dsize + len; >>>> + >>>> + /* This would be a bug */ >>>> + if (dsize >= count) >>>> + return count; >>>> + >>>> + dest += dsize; >>>> + count -= dsize; >>>> + if (len >= count) >>>> + len = count - 1; >>>> + memcpy(dest, src, len); >>>> + dest[len] = 0; >>>> + return res; >>>> +} >>> >>> Why are these needed instead of using strlen and strlcat directly? >> >> Because on powerpc (at least), it will be used in prom_init, it is very >> early in the boot and KASAN shadow memory is not set up yet so calling >> generic string functions would crash the board. > > Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can > you not do something similar? Failing that, I think it would be better to > offer the option for an arch to implement cmdline_*, but have then point to > the normal library routines by default. I don't think it is possible to setup an earlier early shadow. At the point we are in prom_init, the code is not yet relocated at the address it was linked for, and it is running with the MMU set by the bootloader, I can't imagine being able to setup MMU entries for the early shadow KASAN yet without breaking everything. Is it really worth trying to point to the normal library routines by default ? It is really only a few lines of code hence only not many bytes, and anyway they are in __init section so they get discarded at the end. > >>>> +/* >>>> + * This function will append 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. Must not equal dest. >>>> + * @length: the length of dest buffer. >>>> + */ >>>> +static __always_inline void cmdline_build(char *dest, const char *src, size_t length) >>>> +{ >>>> + if (length <= 0) >>>> + return; >>>> + >>>> + dest[0] = 0; >>>> + >>>> +#ifdef CONFIG_CMDLINE >>>> + if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !src || !src[0]) { >>>> + cmdline_strlcat(dest, CONFIG_CMDLINE, length); >>>> + return; >>>> + } >>>> +#endif >>> >>> CONFIG_CMDLINE_FORCE implies CONFIG_CMDLINE, and even if it didn't, >>> CONFIG_CMDLINE is at worst an empty string. Can you drop the #ifdef? >> >> Ah yes, since cbe46bd4f510 ("powerpc: remove CONFIG_CMDLINE #ifdef mess") it >> is feasible. I can change that now. >> >>> >>>> + if (dest != src) >>>> + cmdline_strlcat(dest, src, length); >>>> +#ifdef CONFIG_CMDLINE >>>> + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && sizeof(CONFIG_CMDLINE) > 1) >>>> + cmdline_strlcat(dest, " " CONFIG_CMDLINE, length); >>>> +#endif >>> >>> Likewise, but also I'm not sure why the sizeof() is required. >> >> It is to avoid adding a white space at the end of the command line when >> CONFIG_CMDLINE is empty. But maybe it doesn't matter ? > > If CONFIG_CMDLINE is empty, I don't think you can select > CONFIG_CMDLINE_EXTEND (but even if you could, I don't think it matters). Ok I'll simplify that when I re-spin. Christophe
On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote: > Most architectures have similar boot command line manipulation > options. This patchs adds the definition in init/Kconfig, gated by > CONFIG_HAVE_CMDLINE that the architectures can select to use them. > > In order to use this, a few architectures will have to change their > CONFIG options: > - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER > - architectures using CONFIG_CMDLINE_OVERRIDE or > CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE. > > Architectures also have to define CONFIG_DEFAULT_CMDLINE. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/init/Kconfig b/init/Kconfig > index 22946fe5ded9..a0f2ad9467df 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT > Maximum of each of the number of arguments and environment > variables passed to init from the kernel command line. > > +config HAVE_CMDLINE > + bool > + > +config CMDLINE_BOOL > + bool "Default bootloader kernel arguments" > + depends on HAVE_CMDLINE > + 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. Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter will use CONFIG_CMDLINE if it fails to get anything from the bootloader, which sounds like the same scenario. > +config CMDLINE > + string "Initial kernel command string" s/Initial/Default which is then consistent with the rest of the text here. > + depends on CMDLINE_BOOL Ah, so this is a bit different and I don't think lines-up with the CMDLINE_BOOL help text. > + default DEFAULT_CMDLINE > + 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. (same stale text) > +choice > + prompt "Kernel command line type" if CMDLINE != "" > + default CMDLINE_FROM_BOOTLOADER > + help > + Selects the way you want to use the default kernel arguments. How about: "Determines how the default kernel arguments are combined with any arguments passed by the bootloader" > +config CMDLINE_FROM_BOOTLOADER > + bool "Use bootloader kernel arguments if available" > + help > + Uses the command-line options passed by the boot loader. If > + the boot loader doesn't provide any, the default kernel command > + string provided in CMDLINE will be used. > + > +config CMDLINE_EXTEND Can we rename this to CMDLINE_APPEND, please? There is code in the tree which disagrees about what CMDLINE_EXTEND means, so that will need be to be updated to be consistent (e.g. the EFI stub parsing order). Having the generic option with a different name means we won't accidentally end up with the same inconsistent behaviours. > + bool "Extend bootloader kernel arguments" "Append to the bootloader kernel arguments" > + help > + The default kernel command string will be appended to the > + command-line arguments provided during boot. s/provided during boot/provided by the bootloader/ > + > +config CMDLINE_PREPEND > + bool "Prepend bootloader kernel arguments" "Prepend to the bootloader kernel arguments" > + help > + The default kernel command string will be prepend to the > + command-line arguments provided during boot. s/prepend/prepended/ s/provided during boot/provided by the bootloader/ > + > +config CMDLINE_FORCE > + bool "Always use the default kernel command string" > + 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. I find the "This is useful if ..." sentence really confusing, perhaps just remove it? I'd then tweak it to be: "Always use the default kernel command string, ignoring any arguments provided by the bootloader." Will
Le 03/03/2021 à 18:39, Daniel Walker a écrit : > On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote: >> +Will D >> >> On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote: >>> >>> On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote: >>>> The purpose of this series is to improve and enhance the >>>> handling of kernel boot arguments. >>>> >>>> It is first focussed on powerpc but also extends the capability >>>> for other arches. >>>> >>>> This is based on suggestion from Daniel Walker <danielwa@cisco.com> >>>> >>> >>> >>> I don't see a point in your changes at this time. My changes are much more >>> mature, and you changes don't really make improvements. >> >> Not really a helpful comment. What we merge here will be from whomever >> is persistent and timely in their efforts. But please, work together >> on a common solution. >> >> This one meets my requirements of moving the kconfig and code out of >> the arches, supports prepend/append, and is up to date. > > > Maintainers are capable of merging whatever they want to merge. However, I > wouldn't make hasty choices. The changes I've been submitting have been deployed > on millions of router instances and are more feature rich. > > I believe I worked with you on this change, or something like it, > > https://lkml.org/lkml/2019/3/19/970 > > I don't think Christophe has even addressed this. I thing I have, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3b4291271ce4af4941a771e5af5cbba3c8fa1b2a.1614705851.git.christophe.leroy@csgroup.eu/ If you see something missing in that patch, can you tell me. > I've converted many > architectures, and Cisco uses my changes on at least 4 different > architecture. With products deployed and tested. As far as we know, only powerpc was converted in the last series you submitted, see https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=98106&state=* > > I will resubmit my changes as soon as I can. > Christophe
On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote: > Le 03/03/2021 à 18:46, Will Deacon a écrit : > > On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote: > > > Le 03/03/2021 à 18:28, Will Deacon a écrit : > > > > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: > > > > > This code provides architectures with a way to build command line > > > > > based on what is built in the kernel and what is handed over by the > > > > > bootloader, based on selected compile-time options. > > > > > > > > > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > > > > --- > > > > > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 62 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 000000000000..ae3610bb0ee2 > > > > > --- /dev/null > > > > > +++ b/include/linux/cmdline.h > > > > > @@ -0,0 +1,62 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > +#ifndef _LINUX_CMDLINE_H > > > > > +#define _LINUX_CMDLINE_H > > > > > + > > > > > +static __always_inline size_t cmdline_strlen(const char *s) > > > > > +{ > > > > > + const char *sc; > > > > > + > > > > > + for (sc = s; *sc != '\0'; ++sc) > > > > > + ; /* nothing */ > > > > > + return sc - s; > > > > > +} > > > > > + > > > > > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) > > > > > +{ > > > > > + size_t dsize = cmdline_strlen(dest); > > > > > + size_t len = cmdline_strlen(src); > > > > > + size_t res = dsize + len; > > > > > + > > > > > + /* This would be a bug */ > > > > > + if (dsize >= count) > > > > > + return count; > > > > > + > > > > > + dest += dsize; > > > > > + count -= dsize; > > > > > + if (len >= count) > > > > > + len = count - 1; > > > > > + memcpy(dest, src, len); > > > > > + dest[len] = 0; > > > > > + return res; > > > > > +} > > > > > > > > Why are these needed instead of using strlen and strlcat directly? > > > > > > Because on powerpc (at least), it will be used in prom_init, it is very > > > early in the boot and KASAN shadow memory is not set up yet so calling > > > generic string functions would crash the board. > > > > Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can > > you not do something similar? Failing that, I think it would be better to > > offer the option for an arch to implement cmdline_*, but have then point to > > the normal library routines by default. > > I don't think it is possible to setup an earlier early shadow. > > At the point we are in prom_init, the code is not yet relocated at the > address it was linked for, and it is running with the MMU set by the > bootloader, I can't imagine being able to setup MMU entries for the early > shadow KASAN yet without breaking everything. That's very similar to us; we're not relocated, although we are at least in control of the MMU (which is using a temporary set of page-tables). > Is it really worth trying to point to the normal library routines by default > ? It is really only a few lines of code hence only not many bytes, and > anyway they are in __init section so they get discarded at the end. I would prefer to use the normal routines by default and allow architectures to override them based on their needs, otherwise we'll end up trying to maintain a "lowest-common-dominator" set of string routines that can be safely run in whatever different constraints different architectures have. Will
On Tue, Mar 02, 2021 at 05:25:20PM +0000, Christophe Leroy wrote: > This patchs adds an option of prepend a text to the command > line instead of appending it. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/cmdline.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h > index ae3610bb0ee2..144346051e01 100644 > --- a/include/linux/cmdline.h > +++ b/include/linux/cmdline.h > @@ -31,7 +31,7 @@ static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_ > } > > /* > - * This function will append a builtin command line to the command > + * 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. > @@ -50,6 +50,9 @@ static __always_inline void cmdline_build(char *dest, const char *src, size_t le > cmdline_strlcat(dest, CONFIG_CMDLINE, length); > return; > } > + > + if (IS_ENABLED(CONFIG_CMDLINE_PREPEND) && sizeof(CONFIG_CMDLINE) > 1) > + cmdline_strlcat(dest, CONFIG_CMDLINE " ", length); Same comment as the other patch: I don't think we need to worry about the sizeof() here. Will
On Wed, Mar 03, 2021 at 07:07:45PM +0100, Christophe Leroy wrote: > > > Le 03/03/2021 à 18:39, Daniel Walker a écrit : > > On Tue, Mar 02, 2021 at 08:01:01PM -0600, Rob Herring wrote: > > > +Will D > > > > > > On Tue, Mar 2, 2021 at 11:36 AM Daniel Walker <danielwa@cisco.com> wrote: > > > > > > > > On Tue, Mar 02, 2021 at 05:25:16PM +0000, Christophe Leroy wrote: > > > > > The purpose of this series is to improve and enhance the > > > > > handling of kernel boot arguments. > > > > > > > > > > It is first focussed on powerpc but also extends the capability > > > > > for other arches. > > > > > > > > > > This is based on suggestion from Daniel Walker <danielwa@cisco.com> > > > > > > > > > > > > > > > > > I don't see a point in your changes at this time. My changes are much more > > > > mature, and you changes don't really make improvements. > > > > > > Not really a helpful comment. What we merge here will be from whomever > > > is persistent and timely in their efforts. But please, work together > > > on a common solution. > > > > > > This one meets my requirements of moving the kconfig and code out of > > > the arches, supports prepend/append, and is up to date. > > > > > > Maintainers are capable of merging whatever they want to merge. However, I > > wouldn't make hasty choices. The changes I've been submitting have been deployed > > on millions of router instances and are more feature rich. > > > > I believe I worked with you on this change, or something like it, > > > > https://lkml.org/lkml/2019/3/19/970 > > > > I don't think Christophe has even addressed this. > > I thing I have, see https://patchwork.ozlabs.org/project/linuxppc-dev/patch/3b4291271ce4af4941a771e5af5cbba3c8fa1b2a.1614705851.git.christophe.leroy@csgroup.eu/ > > If you see something missing in that patch, can you tell me. Ok, must have missed that one. > > I've converted many > > architectures, and Cisco uses my changes on at least 4 different > > architecture. With products deployed and tested. > > As far as we know, only powerpc was converted in the last series you > submitted, see > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=98106&state=* Me and others submitted my changes many times, and other architectures have been included. The patch you submitted I've submitted similar at Rob's request years ago. Here a fuller submissions some time ago, https://lore.kernel.org/patchwork/cover/992768/ You've only been involved in prior powerpc only submissions. Daniel
Will Deacon <will@kernel.org> writes: > On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote: >> Le 03/03/2021 à 18:46, Will Deacon a écrit : >> > On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote: >> > > Le 03/03/2021 à 18:28, Will Deacon a écrit : >> > > > On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: >> > > > > This code provides architectures with a way to build command line >> > > > > based on what is built in the kernel and what is handed over by the >> > > > > bootloader, based on selected compile-time options. >> > > > > >> > > > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> > > > > --- >> > > > > include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ >> > > > > 1 file changed, 62 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 000000000000..ae3610bb0ee2 >> > > > > --- /dev/null >> > > > > +++ b/include/linux/cmdline.h >> > > > > @@ -0,0 +1,62 @@ >> > > > > +/* SPDX-License-Identifier: GPL-2.0 */ >> > > > > +#ifndef _LINUX_CMDLINE_H >> > > > > +#define _LINUX_CMDLINE_H >> > > > > + >> > > > > +static __always_inline size_t cmdline_strlen(const char *s) >> > > > > +{ >> > > > > + const char *sc; >> > > > > + >> > > > > + for (sc = s; *sc != '\0'; ++sc) >> > > > > + ; /* nothing */ >> > > > > + return sc - s; >> > > > > +} >> > > > > + >> > > > > +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) >> > > > > +{ >> > > > > + size_t dsize = cmdline_strlen(dest); >> > > > > + size_t len = cmdline_strlen(src); >> > > > > + size_t res = dsize + len; >> > > > > + >> > > > > + /* This would be a bug */ >> > > > > + if (dsize >= count) >> > > > > + return count; >> > > > > + >> > > > > + dest += dsize; >> > > > > + count -= dsize; >> > > > > + if (len >= count) >> > > > > + len = count - 1; >> > > > > + memcpy(dest, src, len); >> > > > > + dest[len] = 0; >> > > > > + return res; >> > > > > +} >> > > > >> > > > Why are these needed instead of using strlen and strlcat directly? >> > > >> > > Because on powerpc (at least), it will be used in prom_init, it is very >> > > early in the boot and KASAN shadow memory is not set up yet so calling >> > > generic string functions would crash the board. >> > >> > Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can >> > you not do something similar? Failing that, I think it would be better to >> > offer the option for an arch to implement cmdline_*, but have then point to >> > the normal library routines by default. >> >> I don't think it is possible to setup an earlier early shadow. >> >> At the point we are in prom_init, the code is not yet relocated at the >> address it was linked for, and it is running with the MMU set by the >> bootloader, I can't imagine being able to setup MMU entries for the early >> shadow KASAN yet without breaking everything. > > That's very similar to us; we're not relocated, although we are at least > in control of the MMU (which is using a temporary set of page-tables). prom_init runs as an OF client, with the MMU off (except on some Apple machines), and we don't own the MMU. So there's really nothing we can do :) Though now that I look at it, I don't think we should be doing this level of commandline handling in prom_init. It should just grab the value from firmware and pass it to the kernel proper, and then all the prepend/append/force etc. logic should happen there. cheers
Le 05/03/2021 à 12:58, Michael Ellerman a écrit : > Will Deacon <will@kernel.org> writes: >> On Wed, Mar 03, 2021 at 06:57:09PM +0100, Christophe Leroy wrote: >>> Le 03/03/2021 à 18:46, Will Deacon a écrit : >>>> On Wed, Mar 03, 2021 at 06:38:16PM +0100, Christophe Leroy wrote: >>>>> Le 03/03/2021 à 18:28, Will Deacon a écrit : >>>>>> On Tue, Mar 02, 2021 at 05:25:17PM +0000, Christophe Leroy wrote: >>>>>>> This code provides architectures with a way to build command line >>>>>>> based on what is built in the kernel and what is handed over by the >>>>>>> bootloader, based on selected compile-time options. >>>>>>> >>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>>>>>> --- >>>>>>> include/linux/cmdline.h | 62 +++++++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 62 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 000000000000..ae3610bb0ee2 >>>>>>> --- /dev/null >>>>>>> +++ b/include/linux/cmdline.h >>>>>>> @@ -0,0 +1,62 @@ >>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>>>> +#ifndef _LINUX_CMDLINE_H >>>>>>> +#define _LINUX_CMDLINE_H >>>>>>> + >>>>>>> +static __always_inline size_t cmdline_strlen(const char *s) >>>>>>> +{ >>>>>>> + const char *sc; >>>>>>> + >>>>>>> + for (sc = s; *sc != '\0'; ++sc) >>>>>>> + ; /* nothing */ >>>>>>> + return sc - s; >>>>>>> +} >>>>>>> + >>>>>>> +static __always_inline size_t cmdline_strlcat(char *dest, const char *src, size_t count) >>>>>>> +{ >>>>>>> + size_t dsize = cmdline_strlen(dest); >>>>>>> + size_t len = cmdline_strlen(src); >>>>>>> + size_t res = dsize + len; >>>>>>> + >>>>>>> + /* This would be a bug */ >>>>>>> + if (dsize >= count) >>>>>>> + return count; >>>>>>> + >>>>>>> + dest += dsize; >>>>>>> + count -= dsize; >>>>>>> + if (len >= count) >>>>>>> + len = count - 1; >>>>>>> + memcpy(dest, src, len); >>>>>>> + dest[len] = 0; >>>>>>> + return res; >>>>>>> +} >>>>>> >>>>>> Why are these needed instead of using strlen and strlcat directly? >>>>> >>>>> Because on powerpc (at least), it will be used in prom_init, it is very >>>>> early in the boot and KASAN shadow memory is not set up yet so calling >>>>> generic string functions would crash the board. >>>> >>>> Hmm. We deliberately setup a _really_ early shadow on arm64 for this, can >>>> you not do something similar? Failing that, I think it would be better to >>>> offer the option for an arch to implement cmdline_*, but have then point to >>>> the normal library routines by default. >>> >>> I don't think it is possible to setup an earlier early shadow. >>> >>> At the point we are in prom_init, the code is not yet relocated at the >>> address it was linked for, and it is running with the MMU set by the >>> bootloader, I can't imagine being able to setup MMU entries for the early >>> shadow KASAN yet without breaking everything. >> >> That's very similar to us; we're not relocated, although we are at least >> in control of the MMU (which is using a temporary set of page-tables). > > prom_init runs as an OF client, with the MMU off (except on some Apple > machines), and we don't own the MMU. So there's really nothing we can do :) > > Though now that I look at it, I don't think we should be doing this > level of commandline handling in prom_init. It should just grab the > value from firmware and pass it to the kernel proper, and then all the > prepend/append/force etc. logic should happen there. But then, how do you handle the command line parameters that are needed by prom_init ? For instance, prom_init_mem() use 'prom_memory_limit', which comes from the "mem=" option in the command line. Christophe
On Fri, Mar 05, 2021 at 10:58:02PM +1100, Michael Ellerman wrote: > Will Deacon <will@kernel.org> writes: > > That's very similar to us; we're not relocated, although we are at least > > in control of the MMU (which is using a temporary set of page-tables). > > prom_init runs as an OF client, with the MMU off (except on some Apple > machines), and we don't own the MMU. So there's really nothing we can do :) You *could* take over all memory mapping. This is complex, and I estimate the change you get this to work correctly on all supported systems to be between -400% and 0%. And not very long later Linux jettisons OF completely anyway. > Though now that I look at it, I don't think we should be doing this > level of commandline handling in prom_init. It should just grab the > value from firmware and pass it to the kernel proper, and then all the > prepend/append/force etc. logic should happen there. That sounds much simpler, yes :-) Segher
On Fri, Mar 05, 2021 at 01:49:03PM +0100, Christophe Leroy wrote: > Le 05/03/2021 à 12:58, Michael Ellerman a écrit : > >prom_init runs as an OF client, with the MMU off (except on some Apple > >machines), and we don't own the MMU. So there's really nothing we can do :) > > > >Though now that I look at it, I don't think we should be doing this > >level of commandline handling in prom_init. It should just grab the > >value from firmware and pass it to the kernel proper, and then all the > >prepend/append/force etc. logic should happen there. > > But then, how do you handle the command line parameters that are needed by > prom_init ? > > For instance, prom_init_mem() use 'prom_memory_limit', which comes from the > "mem=" option in the command line. *Reading* it is easy, much easier than modifying it. Segher
Le 03/03/2021 à 18:57, Will Deacon a écrit : > On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote: >> Most architectures have similar boot command line manipulation >> options. This patchs adds the definition in init/Kconfig, gated by >> CONFIG_HAVE_CMDLINE that the architectures can select to use them. >> >> In order to use this, a few architectures will have to change their >> CONFIG options: >> - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER >> - architectures using CONFIG_CMDLINE_OVERRIDE or >> CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE. >> >> Architectures also have to define CONFIG_DEFAULT_CMDLINE. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/init/Kconfig b/init/Kconfig >> index 22946fe5ded9..a0f2ad9467df 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT >> Maximum of each of the number of arguments and environment >> variables passed to init from the kernel command line. >> >> +config HAVE_CMDLINE >> + bool >> + >> +config CMDLINE_BOOL >> + bool "Default bootloader kernel arguments" >> + depends on HAVE_CMDLINE >> + 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. > > Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter > will use CONFIG_CMDLINE if it fails to get anything from the bootloader, > which sounds like the same scenario. > >> +config CMDLINE >> + string "Initial kernel command string" > > s/Initial/Default > > which is then consistent with the rest of the text here. > >> + depends on CMDLINE_BOOL > > Ah, so this is a bit different and I don't think lines-up with the > CMDLINE_BOOL help text. > >> + default DEFAULT_CMDLINE >> + 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. > > (same stale text) > >> +choice >> + prompt "Kernel command line type" if CMDLINE != "" >> + default CMDLINE_FROM_BOOTLOADER >> + help >> + Selects the way you want to use the default kernel arguments. > > How about: > > "Determines how the default kernel arguments are combined with any > arguments passed by the bootloader" > >> +config CMDLINE_FROM_BOOTLOADER >> + bool "Use bootloader kernel arguments if available" >> + help >> + Uses the command-line options passed by the boot loader. If >> + the boot loader doesn't provide any, the default kernel command >> + string provided in CMDLINE will be used. >> + >> +config CMDLINE_EXTEND > > Can we rename this to CMDLINE_APPEND, please? There is code in the tree > which disagrees about what CMDLINE_EXTEND means, so that will need be > to be updated to be consistent (e.g. the EFI stub parsing order). Having > the generic option with a different name means we won't accidentally end > up with the same inconsistent behaviours. Argh, yes. Seems like the problem is even larger than that IIUC: - For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE - For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments - For SH it means to append the CONFIG_CMDLINE to the bootloader arguments - For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE - For OF it means to append the CONFIG_CMDLINE to the bootloader arguments So what happens on ARM for instance when it selects CONFIG_OF for instance ? Or should we consider that EXTEND means APPEND or PREPEND, no matter which ? Because EXTEND is for instance used for: config INITRAMFS_FORCE bool "Ignore the initramfs passed by the bootloader" depends on CMDLINE_EXTEND || CMDLINE_FORCE Christophe
On Thu, Mar 25, 2021 at 12:18:38PM +0100, Christophe Leroy wrote: > > > Le 03/03/2021 à 18:57, Will Deacon a écrit : > > On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote: > > > Most architectures have similar boot command line manipulation > > > options. This patchs adds the definition in init/Kconfig, gated by > > > CONFIG_HAVE_CMDLINE that the architectures can select to use them. > > > > > > In order to use this, a few architectures will have to change their > > > CONFIG options: > > > - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER > > > - architectures using CONFIG_CMDLINE_OVERRIDE or > > > CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE. > > > > > > Architectures also have to define CONFIG_DEFAULT_CMDLINE. > > > > > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > > --- > > > init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 56 insertions(+) > > > > > > diff --git a/init/Kconfig b/init/Kconfig > > > index 22946fe5ded9..a0f2ad9467df 100644 > > > --- a/init/Kconfig > > > +++ b/init/Kconfig > > > @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT > > > Maximum of each of the number of arguments and environment > > > variables passed to init from the kernel command line. > > > +config HAVE_CMDLINE > > > + bool > > > + > > > +config CMDLINE_BOOL > > > + bool "Default bootloader kernel arguments" > > > + depends on HAVE_CMDLINE > > > + 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. > > > > Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter > > will use CONFIG_CMDLINE if it fails to get anything from the bootloader, > > which sounds like the same scenario. > > > > > +config CMDLINE > > > + string "Initial kernel command string" > > > > s/Initial/Default > > > > which is then consistent with the rest of the text here. > > > > > + depends on CMDLINE_BOOL > > > > Ah, so this is a bit different and I don't think lines-up with the > > CMDLINE_BOOL help text. > > > > > + default DEFAULT_CMDLINE > > > + 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. > > > > (same stale text) > > > > > +choice > > > + prompt "Kernel command line type" if CMDLINE != "" > > > + default CMDLINE_FROM_BOOTLOADER > > > + help > > > + Selects the way you want to use the default kernel arguments. > > > > How about: > > > > "Determines how the default kernel arguments are combined with any > > arguments passed by the bootloader" > > > > > +config CMDLINE_FROM_BOOTLOADER > > > + bool "Use bootloader kernel arguments if available" > > > + help > > > + Uses the command-line options passed by the boot loader. If > > > + the boot loader doesn't provide any, the default kernel command > > > + string provided in CMDLINE will be used. > > > + > > > +config CMDLINE_EXTEND > > > > Can we rename this to CMDLINE_APPEND, please? There is code in the tree > > which disagrees about what CMDLINE_EXTEND means, so that will need be > > to be updated to be consistent (e.g. the EFI stub parsing order). Having > > the generic option with a different name means we won't accidentally end > > up with the same inconsistent behaviours. > > Argh, yes. Seems like the problem is even larger than that IIUC: > > - For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE > - For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments > - For SH it means to append the CONFIG_CMDLINE to the bootloader arguments > - For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE > - For OF it means to append the CONFIG_CMDLINE to the bootloader arguments > > So what happens on ARM for instance when it selects CONFIG_OF for instance ? I think ARM gets different behaviour depending on whether it uses ATAGs or FDT. > Or should we consider that EXTEND means APPEND or PREPEND, no matter which ? > Because EXTEND is for instance used for: > > config INITRAMFS_FORCE > bool "Ignore the initramfs passed by the bootloader" > depends on CMDLINE_EXTEND || CMDLINE_FORCE Oh man, I didn't spot that one :( I think I would make the generic options explicit: either APPEND or PREPEND. Then architectures which choose to define CMDLINE_EXTEND in their Kconfigs can select the generic option that matches their behaviour. INITRAMFS_FORCE sounds like it should depend on APPEND (assuming that means CONFIG_CMDLINE is appended to the bootloader arguments). Will
Le 25/03/2021 à 20:32, Will Deacon a écrit : > On Thu, Mar 25, 2021 at 12:18:38PM +0100, Christophe Leroy wrote: >> >> - For ARM it means to append the bootloader arguments to the CONFIG_CMDLINE >> - For Powerpc it means to append the CONFIG_CMDLINE to the bootloader arguments >> - For SH it means to append the CONFIG_CMDLINE to the bootloader arguments >> - For EFI it means to append the bootloader arguments to the CONFIG_CMDLINE >> - For OF it means to append the CONFIG_CMDLINE to the bootloader arguments >> >> So what happens on ARM for instance when it selects CONFIG_OF for instance ? > > I think ARM gets different behaviour depending on whether it uses ATAGs or > FDT. As far as I can see, ARM uses either ATAGs only or both ATAGs and FDT. ATAGs is forced to 'y' when USE_OF is set. Do I miss something ? > >> Or should we consider that EXTEND means APPEND or PREPEND, no matter which ? >> Because EXTEND is for instance used for: >> >> config INITRAMFS_FORCE >> bool "Ignore the initramfs passed by the bootloader" >> depends on CMDLINE_EXTEND || CMDLINE_FORCE > > Oh man, I didn't spot that one :( > > I think I would make the generic options explicit: either APPEND or PREPEND. > Then architectures which choose to define CMDLINE_EXTEND in their Kconfigs > can select the generic option that matches their behaviour. > > INITRAMFS_FORCE sounds like it should depend on APPEND (assuming that means > CONFIG_CMDLINE is appended to the bootloader arguments). > Christophe
Le 03/03/2021 à 18:57, Will Deacon a écrit : > On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote: >> Most architectures have similar boot command line manipulation >> options. This patchs adds the definition in init/Kconfig, gated by >> CONFIG_HAVE_CMDLINE that the architectures can select to use them. >> >> In order to use this, a few architectures will have to change their >> CONFIG options: >> - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER >> - architectures using CONFIG_CMDLINE_OVERRIDE or >> CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE. >> >> Architectures also have to define CONFIG_DEFAULT_CMDLINE. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/init/Kconfig b/init/Kconfig >> index 22946fe5ded9..a0f2ad9467df 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT >> Maximum of each of the number of arguments and environment >> variables passed to init from the kernel command line. >> >> +config HAVE_CMDLINE >> + bool >> + >> +config CMDLINE_BOOL >> + bool "Default bootloader kernel arguments" >> + depends on HAVE_CMDLINE >> + 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. > > Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter > will use CONFIG_CMDLINE if it fails to get anything from the bootloader, > which sounds like the same scenario. > >> +config CMDLINE >> + string "Initial kernel command string" > > s/Initial/Default > > which is then consistent with the rest of the text here. > >> + depends on CMDLINE_BOOL > > Ah, so this is a bit different and I don't think lines-up with the > CMDLINE_BOOL help text. You are right, the help text is duplicated, I will change the text for the CMDLINE_BOOL > >> + default DEFAULT_CMDLINE >> + 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. > > (same stale text) > >> +choice >> + prompt "Kernel command line type" if CMDLINE != "" >> + default CMDLINE_FROM_BOOTLOADER >> + help >> + Selects the way you want to use the default kernel arguments. > > How about: > > "Determines how the default kernel arguments are combined with any > arguments passed by the bootloader" > >> +config CMDLINE_FROM_BOOTLOADER >> + bool "Use bootloader kernel arguments if available" >> + help >> + Uses the command-line options passed by the boot loader. If >> + the boot loader doesn't provide any, the default kernel command >> + string provided in CMDLINE will be used. >> + >> +config CMDLINE_EXTEND > > Can we rename this to CMDLINE_APPEND, please? There is code in the tree > which disagrees about what CMDLINE_EXTEND means, so that will need be > to be updated to be consistent (e.g. the EFI stub parsing order). Having > the generic option with a different name means we won't accidentally end > up with the same inconsistent behaviours. > >> + bool "Extend bootloader kernel arguments" > > "Append to the bootloader kernel arguments" > >> + help >> + The default kernel command string will be appended to the >> + command-line arguments provided during boot. > > s/provided during boot/provided by the bootloader/ > >> + >> +config CMDLINE_PREPEND >> + bool "Prepend bootloader kernel arguments" > > "Prepend to the bootloader kernel arguments" > >> + help >> + The default kernel command string will be prepend to the >> + command-line arguments provided during boot. > > s/prepend/prepended/ > s/provided during boot/provided by the bootloader/ > >> + >> +config CMDLINE_FORCE >> + bool "Always use the default kernel command string" >> + 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. > > I find the "This is useful if ..." sentence really confusing, perhaps just > remove it? I'd then tweak it to be: > > "Always use the default kernel command string, ignoring any arguments > provided by the bootloader." > Taken all your suggested text. Thanks Christophe
The purpose of this series is to improve and enhance the handling of kernel boot arguments. It is first focussed on powerpc but also extends the capability for other arches. This is based on suggestion from Daniel Walker <danielwa@cisco.com> Christophe Leroy (7): cmdline: Add generic function to build command line. drivers: of: use cmdline building function powerpc: convert to generic builtin command line cmdline: Add capability to prepend the command line powerpc: add capability to prepend default command line cmdline: Gives architectures opportunity to use generically defined boot cmdline manipulation powerpc: use generic CMDLINE manipulations arch/powerpc/Kconfig | 37 ++----------------- arch/powerpc/kernel/prom_init.c | 35 +++--------------- drivers/of/fdt.c | 23 ++---------- include/linux/cmdline.h | 65 +++++++++++++++++++++++++++++++++ init/Kconfig | 56 ++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 83 deletions(-) create mode 100644 include/linux/cmdline.h