Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
On Fri, Mar 05, 2021 at 10:58:02PM +1100, Michael Ellerman wrote: > Will Deacon 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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
Le 05/03/2021 à 12:58, Michael Ellerman a écrit : Will Deacon 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 +, 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 --- 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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
Will Deacon 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 +, 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 >> > > > > --- >> > > > >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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
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 +, 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 > > > > > --- > > > > >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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
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 +, 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 --- 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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
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 +, 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 > > > --- > > > 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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
Le 03/03/2021 à 18:28, Will Deacon a écrit : On Tue, Mar 02, 2021 at 05:25:17PM +, 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 --- 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 ..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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
On Tue, Mar 02, 2021 at 05:25:17PM +, 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 > --- > 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
Re: [PATCH v2 1/7] cmdline: Add generic function to build command line.
On Tue, Mar 02, 2021 at 05:25:17PM +, 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 > --- > 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 ..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
[PATCH v2 1/7] cmdline: Add generic function to build command line.
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 --- 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 ..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; +} + +/* + * 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 + 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 +} + +#endif /* _LINUX_CMDLINE_H */ -- 2.25.0