[{"id":2644189,"web_url":"http://patchwork.ozlabs.org/comment/2644189/","msgid":"<2654902f-c738-b3a2-2f3c-a376cdf1e46d@csgroup.eu>","date":"2021-03-09T07:34:09","subject":"Re: [PATCH v2 1/7] CMDLINE: add generic builtin command line","submitter":{"id":79086,"url":"http://patchwork.ozlabs.org/api/people/79086/","name":"Christophe Leroy","email":"christophe.leroy@csgroup.eu"},"content":"Le 09/03/2021 à 01:02, Daniel Walker a écrit :\n> This code allows architectures to use a generic builtin command line.\n> The state of the builtin command line options across architecture is\n> diverse. On x86 and mips they have pretty much the same code and the\n> code prepends the builtin command line onto the boot loader provided\n> one. On powerpc there is only a builtin override and nothing else.\n\nSame comment as in v1: The above is not correct for powerpc.\n\n> \n> The code in this commit unifies the code into a generic\n> header file under the CONFIG_GENERIC_CMDLINE option. When this\n> option is enabled the architecture can call the cmdline_add_builtin()\n> to add the builtin command line.\n> \n> Cc: xe-linux-external@cisco.com\n> Signed-off-by: Ruslan Bilovol \n> Signed-off-by: Daniel Walker \n> ---\n> include/linux/cmdline.h | 89 +++++++++++++++++++++++++++++++++++++++++\n> init/Kconfig | 68 +++++++++++++++++++++++++++++++\n> 2 files changed, 157 insertions(+)\n> create mode 100644 include/linux/cmdline.h\n> \n> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h\n> new file mode 100644\n> index 000000000000..00929b6e49e6\n> --- /dev/null\n> +++ b/include/linux/cmdline.h\n> @@ -0,0 +1,89 @@\n> +/* SPDX-License-Identifier: GPL-2.0 */\n> +#ifndef _LINUX_CMDLINE_H\n> +#define _LINUX_CMDLINE_H\n> +\n> +/*\n> + *\n> + * Copyright (C) 2006,2021. Cisco Systems, Inc.\n> + *\n> + * Generic Append/Prepend cmdline support.\n> + */\n> +\n> +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)\n> +\n> +#ifndef CONFIG_CMDLINE_OVERRIDE\n> +#define GENERIC_CMDLINE_NEED_STRLCAT\n> +/*\n> + * This function will append or prepend a builtin command line to the command\n> + * line provided by the bootloader. Kconfig options can be used to alter\n> + * the behavior of this builtin command line.\n> + * @dest: The destination of the final appended/prepended string\n> + * @src: The starting string or NULL if there isn't one.\n> + * @tmp: temporary space used for prepending\n> + * @length: the maximum length of the strings above.\n> + * @cmdline_strlcpy: point to a compatible strlcpy\n> + * @cmdline_strlcat: point to a compatible strlcat\n> + */\n> +static inline void\n> +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size),\n> +\t\tsize_t (*cmdline_strlcat)(char *dest, const char *src, size_t count))\n> +{\n> +\tif (src != dest && src != NULL) {\n> +\t\tcmdline_strlcpy(dest, \" \", length);\n> +\t\tcmdline_strlcat(dest, src, length);\n> +\t}\n> +\n> +\tif (sizeof(CONFIG_CMDLINE_APPEND) > 1)\n\nThis test can probably be avoided. if CONFIG_CMDLINE_APPEND is empty, it will add a space at the end \nof dest, that's harmless.\n\n> +\t\tcmdline_strlcat(dest, \" \" CONFIG_CMDLINE_APPEND, length);\n> +\n> +\tif (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {\n\nSame. Keep it simple. Provide tmp all the time, have only one logic.\n\n> +\t\tcmdline_strlcpy(tmp, CONFIG_CMDLINE_PREPEND \" \", length);\n> +\t\tcmdline_strlcat(tmp, dest, length);\n> +\t\tcmdline_strlcpy(dest, tmp, length);\n> +\t}\n> +}\n> +\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n> +\tif (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {\t\t\t\t\t\t\t\t\\\n> +\t\tstatic label char cmdline_tmp_space[length];\t\t\t\t\t\t\t\\\n> +\t\t__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, cmdline_strlcpy, cmdline_strlcat);\t\\\n> +\t} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) {\t\t\t\t\t\t\t\t\\\n> +\t\t__cmdline_add_builtin(dest, src, NULL, length, cmdline_strlcpy, cmdline_strlcat);\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\t\t\t\t\t\\\n\nNo need to micro-optimise this, you can provide cmdline_tmp_space all the time and only keep on leg \nof the if/elseif\n\n> +}\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\tcmdline_add_builtin_custom(dest, src, length, __initdata, strlcpy, strlcat)\n> +\n> +#else /* CONFIG_CMDLINE_OVERRIDE */\n> +\n> +static inline void\n> +__cmdline_add_builtin_custom(char *dest, const char *src, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size))\n\nArgh ! So the same function as different semantics whether CONFIG_CMDLINE_OVERRIDE and/or \nCONFIG_CMDLINE_BOOL is selected ? It means the architecture will have to know it as well in order to \ncall it right ? That looks like micro-optimisation, I think it is not worth it.\n\n\n> +{\n> +\tcmdline_strlcpy(dest, CONFIG_CMDLINE_PREPEND \" \" CONFIG_CMDLINE_APPEND, length);\n> +}\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, cmdline_strlcpy)\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, strlcpy)\n> +#endif /* !CONFIG_CMDLINE_OVERRIDE */\n> +\n> +#else /* !CONFIG_GENERIC_CMDLINE || !CONFIG_CMDLINE_BOOL */\n> +\n> +static inline void\n> +__cmdline_add_builtin_custom(char *dest, const char *src, unsigned long length,\n> +\t\tsize_t (*cmdline_strlcpy)(char *dest, const char *src, size_t size))\n> +{\n> +\tif (src != NULL)\n> +\t\tcmdline_strlcpy(dest, src, length);\n> +}\n> +#define cmdline_add_builtin_custom(dest, src, length, label, cmdline_strlcpy, cmdline_strlcat)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, cmdline_strlcpy)\n> +#define cmdline_add_builtin(dest, src, length)\t\\\n> +\t__cmdline_add_builtin_custom(dest, src, length, strlcpy)\t\\\n> +\n> +#endif /* CONFIG_GENERIC_CMDLINE */\n> +\n> +#endif /* _LINUX_CMDLINE_H */\n> diff --git a/init/Kconfig b/init/Kconfig\n> index 29ad68325028..28363ab07cd4 100644\n> --- a/init/Kconfig\n> +++ b/init/Kconfig\n> @@ -2032,6 +2032,74 @@ config PROFILING\n> config TRACEPOINTS\n> \tbool\n> \n> +config GENERIC_CMDLINE\n> +\tbool\n> +\n> +if GENERIC_CMDLINE\n> +\n> +config CMDLINE_BOOL\n> +\tbool \"Built-in kernel command line\"\n> +\thelp\n> +\t Allow for specifying boot arguments to the kernel at\n> +\t build time. On some systems (e.g. embedded ones), it is\n> +\t necessary or convenient to provide some or all of the\n> +\t kernel boot arguments with the kernel itself (that is,\n> +\t to not rely on the boot loader to provide them.)\n> +\n> +\t To compile command line arguments into the kernel,\n> +\t set this option to 'Y', then fill in the\n> +\t the boot arguments in CONFIG_CMDLINE.\n\nCONFIG_CMDLINE doesn't exist.\n\n> +\n> +\t Systems with fully functional boot loaders (i.e. non-embedded)\n> +\t should leave this option set to 'N'.\n> +\n> +config CMDLINE_APPEND\n> +\tstring \"Built-in kernel command string append\"\n> +\tdepends on CMDLINE_BOOL\n\nI think it would be better to have CMDLINE_APPEND and CMDLINE_PREPENT defined at all time, and not \nleak CMDLINE_BOOL into code. It should just be used here to unable user customisation of \nCMDLINE_APPEND, as follows:\n\nconfig CMDLINE_APPEND\n\tstring \"Built-in kernel command string append\" if CMDLINE_BOOL\n\tdefault \"\"\n\n\n> +\tdefault \"\"\n> +\thelp\n> +\t Enter arguments here that should be compiled into the kernel\n> +\t image and used at boot time. If the boot loader provides a\n> +\t command line at boot time, this string is appended to it to\n> +\t form the full kernel command line, when the system boots.\n> +\n> +\t However, you can use the CONFIG_CMDLINE_OVERRIDE option to\n> +\t change this behavior.\n> +\n> +\t In most cases, the command line (whether built-in or provided\n> +\t by the boot loader) should specify the device for the root\n> +\t file system.\n> +\n> +config CMDLINE_PREPEND\n> +\tstring \"Built-in kernel command string prepend\"\n> +\tdepends on CMDLINE_BOOL\n\nSame comment as for CMDLINE_APPEND\n\n> +\tdefault \"\"\n> +\thelp\n> +\t Enter arguments here that should be compiled into the kernel\n> +\t image and used at boot time. If the boot loader provides a\n> +\t command line at boot time, this string is prepended to it to\n> +\t form the full kernel command line, when the system boots.\n> +\n> +\t However, you can use the CONFIG_CMDLINE_OVERRIDE option to\n> +\t change this behavior.\n> +\n> +\t In most cases, the command line (whether built-in or provided\n> +\t by the boot loader) should specify the device for the root\n> +\t file system.\n> +\n> +config CMDLINE_OVERRIDE\n> +\tbool \"Built-in command line overrides boot loader arguments\"\n> +\tdepends on CMDLINE_BOOL\n> +\thelp\n> +\t Set this option to 'Y' to have the kernel ignore the boot loader\n> +\t command line, and use ONLY the built-in command line. In this case\n> +\t append and prepend strings are concatenated to form the full\n> +\t command line.\n> +\n> +\t This is used to work around broken boot loaders. This should\n> +\t be set to 'N' under normal conditions.\n\nIn powerpc, by default, when a CMDLINE is provided, it is used only if the bootloader doesn't \nprovide a command line. I can't see how I can do that here.\n\n> +endif\n> +\n> endmenu\t\t# General setup\n> \n> source \"arch/Kconfig\"\n>","headers":{"Return-Path":"\n ","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Authentication-Results":["ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org\n (client-ip=2404:9400:2:0:216:3eff:fee1:b9f1; helo=lists.ozlabs.org;\n envelope-from=linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org;\n receiver=)","lists.ozlabs.org; spf=pass (sender SPF authorized)\n smtp.mailfrom=csgroup.eu (client-ip=93.17.236.30; helo=pegase1.c-s.fr;\n envelope-from=christophe.leroy@csgroup.eu; receiver=)"],"Received":["from lists.ozlabs.org (lists.ozlabs.org\n [IPv6:2404:9400:2:0:216:3eff:fee1:b9f1])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 4Dvn6604tkz9sW1\n\tfor ; Tue, 9 Mar 2021 18:34:38 +1100 (AEDT)","from boromir.ozlabs.org (localhost [IPv6:::1])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 4Dvn6568vrz3cHf\n\tfor ; Tue, 9 Mar 2021 18:34:37 +1100 (AEDT)","from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30])\n (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n (No client certificate requested)\n by lists.ozlabs.org (Postfix) with ESMTPS id 4Dvn5n0WJzz30HX\n for ; Tue, 9 Mar 2021 18:34:17 +1100 (AEDT)","from localhost (mailhub1-int [192.168.12.234])\n by localhost (Postfix) with ESMTP id 4Dvn5Y3ZSfz9txlJ;\n Tue, 9 Mar 2021 08:34:09 +0100 (CET)","from pegase1.c-s.fr ([192.168.12.234])\n by localhost (pegase1.c-s.fr [192.168.12.234]) (amavisd-new, port 10024)\n with ESMTP id Ts3OxJuiZwLg; Tue, 9 Mar 2021 08:34:09 +0100 (CET)","from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192])\n by pegase1.c-s.fr (Postfix) with ESMTP id 4Dvn5Y2c3Vz9txlH;\n Tue, 9 Mar 2021 08:34:09 +0100 (CET)","from localhost (localhost [127.0.0.1])\n by messagerie.si.c-s.fr (Postfix) with ESMTP id 6357A8B7CE;\n Tue, 9 Mar 2021 08:34:10 +0100 (CET)","from messagerie.si.c-s.fr ([127.0.0.1])\n by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023)\n with ESMTP id vEw3QlpfrpDT; Tue, 9 Mar 2021 08:34:10 +0100 (CET)","from [192.168.4.90] (unknown [192.168.4.90])\n by messagerie.si.c-s.fr (Postfix) with ESMTP id 885058B773;\n Tue, 9 Mar 2021 08:34:09 +0100 (CET)"],"X-Virus-Scanned":["Debian amavisd-new at c-s.fr","amavisd-new at c-s.fr"],"Subject":"Re: [PATCH v2 1/7] CMDLINE: add generic builtin command line","To":"Daniel Walker , Will Deacon ,\n Rob Herring ,\n Daniel Gimpelevich ,\n Andrew Morton , x86@kernel.org,\n linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,\n xe-linux-external@cisco.com","References":"<20210309000247.2989531-2-danielwa@cisco.com>","From":"Christophe Leroy ","Message-ID":"<2654902f-c738-b3a2-2f3c-a376cdf1e46d@csgroup.eu>","Date":"Tue, 9 Mar 2021 08:34:09 +0100","User-Agent":"Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:78.0) Gecko/20100101\n Thunderbird/78.8.0","MIME-Version":"1.0","In-Reply-To":"<20210309000247.2989531-2-danielwa@cisco.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"fr","Content-Transfer-Encoding":"8bit","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List ","List-Unsubscribe":",\n ","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n ","Cc":"Ruslan Bilovol , linux-kernel@vger.kernel.org","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n "}}]