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