[2/2] bootconfig: Apply early options from embedded config

  Eliminate all allocations in embedded config handling to allow calling
it from arch_setup and applying early options.

Config stored in initrd can't be used for early options, because initrd
is set up after early options are processed.

Add this information to the documentation and also to the option
description.

Signed-off-by: Petr Malat <oss@malat.biz>
---
 Documentation/admin-guide/bootconfig.rst |   3 +
 init/Kconfig                             |   4 +-
 init/main.c                              | 141 ++++++++++++++++++-----
 lib/bootconfig.c                         |  20 +++-
 4 files changed, 132 insertions(+), 36 deletions(-)
  

Hi--

On 11/21/23 15:13, Petr Malat wrote:
> Eliminate all allocations in embedded config handling to allow calling
> it from arch_setup and applying early options.
> 
> Config stored in initrd can't be used for early options, because initrd
> is set up after early options are processed.
> 
> Add this information to the documentation and also to the option
> description.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  Documentation/admin-guide/bootconfig.rst |   3 +
>  init/Kconfig                             |   4 +-
>  init/main.c                              | 141 ++++++++++++++++++-----
>  lib/bootconfig.c                         |  20 +++-
>  4 files changed, 132 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index 91339efdcb54..fb085f696f9b 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
>  There are two options to boot the kernel with bootconfig: attaching the
>  bootconfig to the initrd image or embedding it in the kernel itself.
>  
> +Early options may be specified only in the embedded bootconfig, because
> +they are processed before the initrd.
> +

I'm confused by which options are early options. Are they specified or
described somewhere?
How does a user know which options are "early" options?


>  Attaching a Boot Config to Initrd
>  ---------------------------------
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 9161d2dbad0c..04de756c935e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
>  	  Extra boot config allows system admin to pass a config file as
>  	  complemental extension of kernel cmdline when booting.
>  	  The boot config file must be attached at the end of initramfs
> -	  with checksum, size and magic word.
> +	  with checksum, size and magic word. Note that early options may
> +	  be specified in the embedded bootconfig only. Early options
> +	  specified in initrd bootconfig will not be applied.
>  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
>  
>  	  If unsure, say Y.
> diff --git a/init/main.c b/init/main.c
> index 0cd738f7f0cf..9aac59673a3a 100644
> --- a/init/main.c
> +++ b/init/main.c

[]

> +
> +static int __init setup_boot_config_early(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> -	const char *msg, *initrd_data;
> -	int pos, ret;
> -	size_t initrd_size, embeded_size = 0;
> -	char *err, *embeded_data = NULL;
> +	static int prev_rtn __initdata;
> +	struct xbc_node *root, *knode, *vnode;
> +	char *embeded_data, *err;
> +	const char *val, *msg;
> +	size_t embeded_size;
> +	int ret, pos;

It hurts my eyes to see "embeded" here.

Thanks.
  

On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> Hi--
> 
> On 11/21/23 15:13, Petr Malat wrote:
> > Eliminate all allocations in embedded config handling to allow calling
> > it from arch_setup and applying early options.
> > 
> > Config stored in initrd can't be used for early options, because initrd
> > is set up after early options are processed.
> > 
> > Add this information to the documentation and also to the option
> > description.
> > 
> > Signed-off-by: Petr Malat <oss@malat.biz>
> > ---
> >  Documentation/admin-guide/bootconfig.rst |   3 +
> >  init/Kconfig                             |   4 +-
> >  init/main.c                              | 141 ++++++++++++++++++-----
> >  lib/bootconfig.c                         |  20 +++-
> >  4 files changed, 132 insertions(+), 36 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > index 91339efdcb54..fb085f696f9b 100644
> > --- a/Documentation/admin-guide/bootconfig.rst
> > +++ b/Documentation/admin-guide/bootconfig.rst
> > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> >  There are two options to boot the kernel with bootconfig: attaching the
> >  bootconfig to the initrd image or embedding it in the kernel itself.
> >  
> > +Early options may be specified only in the embedded bootconfig, because
> > +they are processed before the initrd.
> > +
> 
> I'm confused by which options are early options. Are they specified or
> described somewhere?
> How does a user know which options are "early" options?

I don't claim to know the full answer to this question, but those
declared with early_param() are ones that I have run into.  There are
a few hundred of these.  I believe that there are at least a few more
where the parsing is done manually directly out of the buffer, and
some of those might well also qualify as "early".

Would it make sense to add an "EARLY" to the long list of restrictions
in Documentation/admin-guide/kernel-parameters.rst?

							Thanx, Paul

> >  Attaching a Boot Config to Initrd
> >  ---------------------------------
> >  
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 9161d2dbad0c..04de756c935e 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> >  	  Extra boot config allows system admin to pass a config file as
> >  	  complemental extension of kernel cmdline when booting.
> >  	  The boot config file must be attached at the end of initramfs
> > -	  with checksum, size and magic word.
> > +	  with checksum, size and magic word. Note that early options may
> > +	  be specified in the embedded bootconfig only. Early options
> > +	  specified in initrd bootconfig will not be applied.
> >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> >  
> >  	  If unsure, say Y.
> > diff --git a/init/main.c b/init/main.c
> > index 0cd738f7f0cf..9aac59673a3a 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> 
> []
> 
> > +
> > +static int __init setup_boot_config_early(void)
> >  {
> >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > -	const char *msg, *initrd_data;
> > -	int pos, ret;
> > -	size_t initrd_size, embeded_size = 0;
> > -	char *err, *embeded_data = NULL;
> > +	static int prev_rtn __initdata;
> > +	struct xbc_node *root, *knode, *vnode;
> > +	char *embeded_data, *err;
> > +	const char *val, *msg;
> > +	size_t embeded_size;
> > +	int ret, pos;
> 
> It hurts my eyes to see "embeded" here.
> 
> Thanks.
> -- 
> ~Randy
  

On Wed, 22 Nov 2023 15:38:03 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > Hi--
> > 
> > On 11/21/23 15:13, Petr Malat wrote:
> > > Eliminate all allocations in embedded config handling to allow calling
> > > it from arch_setup and applying early options.
> > > 
> > > Config stored in initrd can't be used for early options, because initrd
> > > is set up after early options are processed.
> > > 
> > > Add this information to the documentation and also to the option
> > > description.
> > > 
> > > Signed-off-by: Petr Malat <oss@malat.biz>
> > > ---
> > >  Documentation/admin-guide/bootconfig.rst |   3 +
> > >  init/Kconfig                             |   4 +-
> > >  init/main.c                              | 141 ++++++++++++++++++-----
> > >  lib/bootconfig.c                         |  20 +++-
> > >  4 files changed, 132 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > > index 91339efdcb54..fb085f696f9b 100644
> > > --- a/Documentation/admin-guide/bootconfig.rst
> > > +++ b/Documentation/admin-guide/bootconfig.rst
> > > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> > >  There are two options to boot the kernel with bootconfig: attaching the
> > >  bootconfig to the initrd image or embedding it in the kernel itself.
> > >  
> > > +Early options may be specified only in the embedded bootconfig, because
> > > +they are processed before the initrd.
> > > +
> > 
> > I'm confused by which options are early options. Are they specified or
> > described somewhere?
> > How does a user know which options are "early" options?
> 
> I don't claim to know the full answer to this question, but those
> declared with early_param() are ones that I have run into.  There are
> a few hundred of these.  I believe that there are at least a few more
> where the parsing is done manually directly out of the buffer, and
> some of those might well also qualify as "early".
> 
> Would it make sense to add an "EARLY" to the long list of restrictions
> in Documentation/admin-guide/kernel-parameters.rst?

Agreed. For the bootconfig, we need to do this...

BTW, we also need to make a block-list for some early params. some of those
MUST be passed from the bootloader. E.g. initrd address and size will be
passed from the bootloader via commandline. Thus such params in the embedded
bootconfig should be filtered at the build time.

Thank you,

> 
> 							Thanx, Paul
> 
> > >  Attaching a Boot Config to Initrd
> > >  ---------------------------------
> > >  
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index 9161d2dbad0c..04de756c935e 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> > >  	  Extra boot config allows system admin to pass a config file as
> > >  	  complemental extension of kernel cmdline when booting.
> > >  	  The boot config file must be attached at the end of initramfs
> > > -	  with checksum, size and magic word.
> > > +	  with checksum, size and magic word. Note that early options may
> > > +	  be specified in the embedded bootconfig only. Early options
> > > +	  specified in initrd bootconfig will not be applied.
> > >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> > >  
> > >  	  If unsure, say Y.
> > > diff --git a/init/main.c b/init/main.c
> > > index 0cd738f7f0cf..9aac59673a3a 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > 
> > []
> > 
> > > +
> > > +static int __init setup_boot_config_early(void)
> > >  {
> > >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > > -	const char *msg, *initrd_data;
> > > -	int pos, ret;
> > > -	size_t initrd_size, embeded_size = 0;
> > > -	char *err, *embeded_data = NULL;
> > > +	static int prev_rtn __initdata;
> > > +	struct xbc_node *root, *knode, *vnode;
> > > +	char *embeded_data, *err;
> > > +	const char *val, *msg;
> > > +	size_t embeded_size;
> > > +	int ret, pos;
> > 
> > It hurts my eyes to see "embeded" here.
> > 
> > Thanks.
> > -- 
> > ~Randy
  

Hi!

On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > +Early options may be specified only in the embedded bootconfig, because
> > +they are processed before the initrd.
> > +
> 
> I'm confused by which options are early options. Are they specified or
> described somewhere?
> How does a user know which options are "early" options?
This is not user friendly at all, I was thinking about emitting a warning
when early option is being configured from initrd, but that would require
one iteration over .init.setup section for every option present in initrd
just to show the warning.

Better idea would be to write a script, which extracts this info from
vmlinux.o and adds it to bootconfig userspace utility. Of course this makes
the utility tied to particular kernel version and configuration
combination, but for distributions it's reasonably safe with no overhead
during boot.

> > +	size_t embeded_size;
> > +	int ret, pos;
> 
> It hurts my eyes to see "embeded" here.
Thanks, I will fix it in the next version.
  Petr
  

Hi!

On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> BTW, we also need to make a block-list for some early params. some of those
> MUST be passed from the bootloader. E.g. initrd address and size will be
> passed from the bootloader via commandline. Thus such params in the embedded
> bootconfig should be filtered at the build time.

It's ok to configure these in the embedded bootconfig - in a case they are
provided by the bootloader, the bootconfig value is overridden, if not, the
value from bootconfig is used, so it works as expected.
  Petr
  

On Wed, 22 Nov 2023 00:13:42 +0100
Petr Malat <oss@malat.biz> wrote:

> Eliminate all allocations in embedded config handling to allow calling
> it from arch_setup and applying early options.
> 
> Config stored in initrd can't be used for early options, because initrd
> is set up after early options are processed.
> 
> Add this information to the documentation and also to the option
> description.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  Documentation/admin-guide/bootconfig.rst |   3 +
>  init/Kconfig                             |   4 +-
>  init/main.c                              | 141 ++++++++++++++++++-----
>  lib/bootconfig.c                         |  20 +++-
>  4 files changed, 132 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index 91339efdcb54..fb085f696f9b 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
>  There are two options to boot the kernel with bootconfig: attaching the
>  bootconfig to the initrd image or embedding it in the kernel itself.
>  
> +Early options may be specified only in the embedded bootconfig, because
> +they are processed before the initrd.
> +
>  Attaching a Boot Config to Initrd
>  ---------------------------------
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 9161d2dbad0c..04de756c935e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
>  	  Extra boot config allows system admin to pass a config file as
>  	  complemental extension of kernel cmdline when booting.
>  	  The boot config file must be attached at the end of initramfs
> -	  with checksum, size and magic word.
> +	  with checksum, size and magic word. Note that early options may
> +	  be specified in the embedded bootconfig only. Early options
> +	  specified in initrd bootconfig will not be applied.
>  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
>  
>  	  If unsure, say Y.
> diff --git a/init/main.c b/init/main.c
> index 0cd738f7f0cf..9aac59673a3a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -158,6 +158,9 @@ static size_t initargs_offs;
>  static char *execute_command;
>  static char *ramdisk_execute_command = "/init";
>  
> +static int __init do_early_param(char *param, char *val,
> +				 const char *unused, void *arg);
> +
>  /*
>   * Used to generate warnings if static_key manipulation functions are used
>   * before jump_label_init is called.
> @@ -406,63 +409,134 @@ static int __init warn_bootconfig(char *str)
>  	return 0;
>  }
>  
> -static void __init setup_boot_config(void)
> +static void __init boot_config_pr_err(const char *msg, int pos, const char *src)
> +{
> +	if (pos < 0)
> +		pr_err("Failed to init bootconfig: %s.\n", msg);
> +	else
> +		pr_err("Failed to parse %s bootconfig: %s at %d.\n",
> +				src, msg, pos);
> +}
> +
> +static int __init setup_boot_config_early(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> -	const char *msg, *initrd_data;
> -	int pos, ret;
> -	size_t initrd_size, embeded_size = 0;
> -	char *err, *embeded_data = NULL;
> +	static int prev_rtn __initdata;

I would rather like 'done_retval' instead of 'prev_rtn'.

> +	struct xbc_node *root, *knode, *vnode;
> +	char *embeded_data, *err;
> +	const char *val, *msg;
> +	size_t embeded_size;
> +	int ret, pos;
>  
> -	/* Cut out the bootconfig data even if we have no bootconfig option */
> -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> -	/* If there is no bootconfig in initrd, try embedded one. */
> -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> +	if (prev_rtn)
> +		return prev_rtn;
>  
> +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
>  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
>  			 bootconfig_params);

This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
this only checks "bootconfig" is in the cmdline.
Can you introduce following flag and initialize it here?

#ifdef CONFIG_BOOT_CONFIG_FORCE
#define bootconfig_enabled	(true)
#else
static bool bootconfig_enabled __initdata;
#endif

> -
> -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> -		return;
> -
> +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> +		return prev_rtn;

Than we don't need to set a strange error value...

> +	}
>  	/* parse_args() stops at the next param of '--' and returns an address */
>  	if (err)
>  		initargs_offs = err - tmp_cmdline;
>  
> -	if (!initrd_data && !embeded_data) {
> -		/* If user intended to use bootconfig, show an error level message */
> -		if (bootconfig_found)
> -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> -		else
> -			pr_info("No bootconfig data provided, so skipping bootconfig");
> -		return;
> +	if (!embeded_data) {
> +		prev_rtn = -ENOPROTOOPT;

Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
using this error code.

> +		return prev_rtn;
>  	}
>  
>  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> -	if (ret < 0)
> -		goto err0;
> +	if (ret < 0) {
> +		boot_config_pr_err(msg, pos, "embedded");
> +		prev_rtn = ret;
> +		return prev_rtn;
> +	}
> +	prev_rtn = 1;

This function should be splitted into init_embedded_boot_config() and
apply_boot_config_early(). The latter one should not be called twice.

> +
> +	/* Process early options */
> +	root = xbc_find_node("kernel");
> +	if (!root)
> +		goto out;
> +
> +	xbc_node_for_each_key_value(root, knode, val) {
> +		ret = xbc_node_compose_key_after(root, knode,
> +				xbc_namebuf, XBC_KEYLEN_MAX);
> +		if (ret < 0)
> +			continue;
> +
> +		vnode = xbc_node_get_child(knode);
> +		if (!vnode) {
> +			do_early_param(xbc_namebuf, NULL, NULL, NULL);
> +			continue;
> +		}
> +
> +		xbc_array_for_each_value(vnode, val) {
> +			if (strscpy(tmp_cmdline, val, sizeof(tmp_cmdline)) < 1) {
> +				pr_err("Value for '%s' too long\n", xbc_namebuf);
> +				break;
> +			}
> +			do_early_param(xbc_namebuf, tmp_cmdline, NULL, NULL);
> +		}
> +	}
> +
> +out:	return embeded_data ? 1 : 0;
> +}
> +
> +static void __init setup_boot_config(void)
> +{
> +	const char *msg, *initrd_data;
> +	int pos, ret;
> +	size_t initrd_size, s;
> +
> +	/* Cut out the bootconfig data even if we have no bootconfig option */
> +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> +
> +	ret = setup_boot_config_early();

Because you should not apply early_params here, you need to call only
init_embedded_boot_config() here.

> +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {

Also, this can be
	if (!bootconfig_enabled) {
		if (initrd_data || xbc_get_embedded_bootconfig(&s))

> +		pr_info("Bootconfig data present, but handling is disabled\n");
> +		return;


> +	} else if (ret == -ENODATA) {
> +		/* Bootconfig disabled and bootconfig data are not present */
> +		return;

this can be removed.

> +	} else if (ret == -ENOPROTOOPT) {

This should be

	} else {

> +		/* Embedded bootconfig not found */
> +		if (!initrd_data) {
> +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> +			return;
> +		}
> +		ret = xbc_init(NULL, 0, &msg, &pos);
> +		if (ret)
> +			goto err0;

> +	} else if (ret < 0) {
> +		/* Other error, should be logged already */
> +		return;

So this is checked at first.

> +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {

And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
should be removed.

> +		/* Embedded bootconfig handled, but we should not append to it */
> +		xbc_get_info(&ret, &s);
> +		pr_info("Replacing embedded bootconfig of %d nodes and %zu bytes.\n", ret, s);
> +		xbc_exit();
> +		ret = xbc_init(NULL, 0, &msg, &pos);
> +		if (ret)
> +			goto err0;
> +	}

Thank you,

>  
>  	/* Call append even if no data are there as embedded bootconfig is in .init */
>  	ret = xbc_append(initrd_data, initrd_size, &msg, &pos);
>  	if (ret < 0)
>  		goto err0;
>  
> -	xbc_get_info(&ret, NULL);
> -	pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)(embeded_size + initrd_size), ret);
> +	xbc_get_info(&ret, &s);
> +	pr_info("Load bootconfig: %d nodes %zu bytes.\n", ret, s);
>  	/* keys starting with "kernel." are passed via cmdline */
>  	extra_command_line = xbc_make_cmdline("kernel");
>  	/* Also, "init." keys are init arguments */
>  	extra_init_args = xbc_make_cmdline("init");
>  	return;
>  
> -err0:	if (pos < 0)
> -		pr_err("Failed to init bootconfig: %s.\n", msg);
> -	else
> -		pr_err("Failed to parse %s bootconfig: %s at %zu.\n",
> -				pos < embeded_size ? "embedded" : "initrd",
> -				msg, pos < embeded_size ? pos : pos - embeded_size);
> +err0:	boot_config_pr_err(msg, pos, "initrd");
>  }
>  
>  static void __init exit_boot_config(void)
> @@ -766,6 +840,11 @@ void __init parse_early_param(void)
>  	if (done)
>  		return;
>  
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +	/* Process early options from boot config */
> +	setup_boot_config_early();
> +#endif
> +
>  	/* All fall through to do_early_param. */
>  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  	parse_early_options(tmp_cmdline);
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index 841163ce5313..4048057e3e23 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -45,7 +45,11 @@ char * __init xbc_get_embedded_bootconfig(size_t *size)
>   * node (for array).
>   */
>  
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +static struct xbc_node xbc_nodes[XBC_NODE_MAX];
> +#else
>  static struct xbc_node *xbc_nodes __initdata;
> +#endif
>  static int xbc_node_num __initdata;
>  static char *xbc_data __initdata;
>  static bool xbc_data_allocated __initdata;
> @@ -914,8 +918,12 @@ void __init xbc_exit(void)
>  	xbc_data_size = 0;
>  	xbc_data_allocated = 0;
>  	xbc_node_num = 0;
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +	memset(xbc_nodes, 0, sizeof(xbc_nodes));
> +#else
>  	xbc_free_mem(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	xbc_nodes = NULL;
> +#endif
>  	brace_index = 0;
>  }
>  
> @@ -973,6 +981,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
>  		return -ERANGE;
>  	}
>  
> +#ifndef CONFIG_BOOT_CONFIG_EMBED
>  	xbc_nodes = xbc_alloc_mem(sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	if (!xbc_nodes) {
>  		if (emsg)
> @@ -980,7 +989,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
>  		return -ENOMEM;
>  	}
>  	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
> -
> +#endif
>  	if (!data)
>  		return 0;
>  	xbc_data = data;
> @@ -999,6 +1008,7 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  {
>  	size_t new_size, parse_start;
>  	char *new_data;
> +	int ret;
>  
>  	new_size = xbc_data_size + size;
>  	if (new_size > XBC_DATA_MAX) {
> @@ -1024,8 +1034,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  
>  	if (xbc_data_size) {
>  		memcpy(new_data, xbc_data, xbc_data_size - 1);
> -		new_data[xbc_data_size - 1] = '\n';
>  		parse_start = xbc_data_size - 1;
> +		new_data[parse_start] = '\n';
>  	} else {
>  		parse_start = 0;
>  	}
> @@ -1039,6 +1049,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  
>  	if (!data)
>  		return 0;
> -
> -	return xbc_parse_and_verify_tree(parse_start, epos, emsg);
> +	ret = xbc_parse_and_verify_tree(parse_start, epos, emsg);
> +	if (ret && epos)
> +		*epos -= parse_start;
> +	return ret;
>  }
> -- 
> 2.30.2
>
  

On Thu, 23 Nov 2023 11:04:00 +0100
Petr Malat <oss@malat.biz> wrote:

> Hi!
> 
> On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> > BTW, we also need to make a block-list for some early params. some of those
> > MUST be passed from the bootloader. E.g. initrd address and size will be
> > passed from the bootloader via commandline. Thus such params in the embedded
> > bootconfig should be filtered at the build time.
> 
> It's ok to configure these in the embedded bootconfig - in a case they are
> provided by the bootloader, the bootconfig value is overridden, if not, the
> value from bootconfig is used, so it works as expected.

I meant some params only bootloader knows, like where the initrd is loaded.
Anyway, if user sets such value, it will break the kernel boot as expected :P.

Thank you,

>   Petr
  

On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> On Wed, 22 Nov 2023 15:38:03 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > > Hi--
> > > 
> > > On 11/21/23 15:13, Petr Malat wrote:
> > > > Eliminate all allocations in embedded config handling to allow calling
> > > > it from arch_setup and applying early options.
> > > > 
> > > > Config stored in initrd can't be used for early options, because initrd
> > > > is set up after early options are processed.
> > > > 
> > > > Add this information to the documentation and also to the option
> > > > description.
> > > > 
> > > > Signed-off-by: Petr Malat <oss@malat.biz>
> > > > ---
> > > >  Documentation/admin-guide/bootconfig.rst |   3 +
> > > >  init/Kconfig                             |   4 +-
> > > >  init/main.c                              | 141 ++++++++++++++++++-----
> > > >  lib/bootconfig.c                         |  20 +++-
> > > >  4 files changed, 132 insertions(+), 36 deletions(-)
> > > > 
> > > > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > > > index 91339efdcb54..fb085f696f9b 100644
> > > > --- a/Documentation/admin-guide/bootconfig.rst
> > > > +++ b/Documentation/admin-guide/bootconfig.rst
> > > > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> > > >  There are two options to boot the kernel with bootconfig: attaching the
> > > >  bootconfig to the initrd image or embedding it in the kernel itself.
> > > >  
> > > > +Early options may be specified only in the embedded bootconfig, because
> > > > +they are processed before the initrd.
> > > > +
> > > 
> > > I'm confused by which options are early options. Are they specified or
> > > described somewhere?
> > > How does a user know which options are "early" options?
> > 
> > I don't claim to know the full answer to this question, but those
> > declared with early_param() are ones that I have run into.  There are
> > a few hundred of these.  I believe that there are at least a few more
> > where the parsing is done manually directly out of the buffer, and
> > some of those might well also qualify as "early".
> > 
> > Would it make sense to add an "EARLY" to the long list of restrictions
> > in Documentation/admin-guide/kernel-parameters.rst?
> 
> Agreed. For the bootconfig, we need to do this...

Very good, I will put something together by early next week.

There will likely be early_setup() parameters that are not documented
in kernel-parameters.txt.  I propose leaving those undocumented, but
upgrading the header comment on early_param() to explain the situation.

Seem reasonable, or would something else work better?

> BTW, we also need to make a block-list for some early params. some of those
> MUST be passed from the bootloader. E.g. initrd address and size will be
> passed from the bootloader via commandline. Thus such params in the embedded
> bootconfig should be filtered at the build time.

Given a list of them, I would be happy to generate the patches to the
documentation.

							Thanx, Paul

> > > >  Attaching a Boot Config to Initrd
> > > >  ---------------------------------
> > > >  
> > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > index 9161d2dbad0c..04de756c935e 100644
> > > > --- a/init/Kconfig
> > > > +++ b/init/Kconfig
> > > > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> > > >  	  Extra boot config allows system admin to pass a config file as
> > > >  	  complemental extension of kernel cmdline when booting.
> > > >  	  The boot config file must be attached at the end of initramfs
> > > > -	  with checksum, size and magic word.
> > > > +	  with checksum, size and magic word. Note that early options may
> > > > +	  be specified in the embedded bootconfig only. Early options
> > > > +	  specified in initrd bootconfig will not be applied.
> > > >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> > > >  
> > > >  	  If unsure, say Y.
> > > > diff --git a/init/main.c b/init/main.c
> > > > index 0cd738f7f0cf..9aac59673a3a 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > 
> > > []
> > > 
> > > > +
> > > > +static int __init setup_boot_config_early(void)
> > > >  {
> > > >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > > > -	const char *msg, *initrd_data;
> > > > -	int pos, ret;
> > > > -	size_t initrd_size, embeded_size = 0;
> > > > -	char *err, *embeded_data = NULL;
> > > > +	static int prev_rtn __initdata;
> > > > +	struct xbc_node *root, *knode, *vnode;
> > > > +	char *embeded_data, *err;
> > > > +	const char *val, *msg;
> > > > +	size_t embeded_size;
> > > > +	int ret, pos;
> > > 
> > > It hurts my eyes to see "embeded" here.
> > > 
> > > Thanks.
> > > -- 
> > > ~Randy
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
  

On Thu, Nov 23, 2023 at 07:41:06PM +0900, Masami Hiramatsu wrote:
> On Wed, 22 Nov 2023 00:13:42 +0100
> Petr Malat <oss@malat.biz> wrote:
> > +	static int prev_rtn __initdata;
> 
> I would rather like 'done_retval' instead of 'prev_rtn'.
 
OK, I will rename it.

> > -	/* Cut out the bootconfig data even if we have no bootconfig option */
> > -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > -	/* If there is no bootconfig in initrd, try embedded one. */
> > -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> > -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > +	if (prev_rtn)
> > +		return prev_rtn;
> >  
> > +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> >  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> >  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> >  			 bootconfig_params);
> 
> This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
> this only checks "bootconfig" is in the cmdline.
> Can you introduce following flag and initialize it here?
> 
> #ifdef CONFIG_BOOT_CONFIG_FORCE
> #define bootconfig_enabled	(true)
> #else
> static bool bootconfig_enabled __initdata;
> #endif

Even when CONFIG_BOOT_CONFIG_FORCE is set, we must call parse_args to find
the location of -- to know where init options should be added. It's done the
same way in the current code.

 
> > -
> > -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> > -		return;
> > -
> > +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> > +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> > +		return prev_rtn;
> 
> Than we don't need to set a strange error value...

It's used for error logging as the current code emits a different
messages in these situations, but I will try to refactor it.


> > +	}
> >  	/* parse_args() stops at the next param of '--' and returns an address */
> >  	if (err)
> >  		initargs_offs = err - tmp_cmdline;
> >  
> > -	if (!initrd_data && !embeded_data) {
> > -		/* If user intended to use bootconfig, show an error level message */
> > -		if (bootconfig_found)
> > -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> > -		else
> > -			pr_info("No bootconfig data provided, so skipping bootconfig");
> > -		return;
> > +	if (!embeded_data) {
> > +		prev_rtn = -ENOPROTOOPT;
> 
> Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
> using this error code.

ok, I will try to refactor the error logging. Calling
xbc_get_embedded_bootconfig is cheap.

> > +		return prev_rtn;
> >  	}
> >  
> >  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> > -	if (ret < 0)
> > -		goto err0;
> > +	if (ret < 0) {
> > +		boot_config_pr_err(msg, pos, "embedded");
> > +		prev_rtn = ret;
> > +		return prev_rtn;
> > +	}
> > +	prev_rtn = 1;
> 
> This function should be splitted into init_embedded_boot_config() and
> apply_boot_config_early(). The latter one should not be called twice.
>
> > .....
> > +
> > +static void __init setup_boot_config(void)
> > +{
> > +	const char *msg, *initrd_data;
> > +	int pos, ret;
> > +	size_t initrd_size, s;
> > +
> > +	/* Cut out the bootconfig data even if we have no bootconfig option */
> > +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > +
> > +	ret = setup_boot_config_early();
> 
> Because you should not apply early_params here, you need to call only
> init_embedded_boot_config() here.

setup_boot_config_early() must be called from 2 places, because there is
no guarantee the architecture specific code calls parse_early_param() - it's
not mandatory. If it's not called by architecture, it's called quite late
by start_kernel(), later than setup_boot_config().
I want to avoid different behavior on different architectures, so I always
process early options in the embedded config only, although on some
architectures even these from initrd could be used, but it could cause
issues in the future if the architecture would need to switch.


> > +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {
> 
> Also, this can be
> 	if (!bootconfig_enabled) {
> 		if (initrd_data || xbc_get_embedded_bootconfig(&s))
> 
> > +		pr_info("Bootconfig data present, but handling is disabled\n");
> > +		return;
> 
> 
> > +	} else if (ret == -ENODATA) {
> > +		/* Bootconfig disabled and bootconfig data are not present */
> > +		return;
> 
> this can be removed.
> 
> > +	} else if (ret == -ENOPROTOOPT) {
> 
> This should be
> 
> 	} else {
> 
> > +		/* Embedded bootconfig not found */
> > +		if (!initrd_data) {
> > +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> > +			return;
> > +		}
> > +		ret = xbc_init(NULL, 0, &msg, &pos);
> > +		if (ret)
> > +			goto err0;
> 
> > +	} else if (ret < 0) {
> > +		/* Other error, should be logged already */
> > +		return;
> 
> So this is checked at first.
> 
> > +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {
> 
> And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> should be removed.

I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
compatible change and I didn't want to risk breaking current use cases.
My change tries to get early options working without affecting how
other options are handled, but I think appending the config is more
reasonable behavior and if you do not see it as a problem to not be
backward compatible here, I will delete the "replace" behavior.

I will try to refactor the error handling.
  Petr
  

Hi!
On Thu, Nov 23, 2023 at 11:18:05PM +0900, Masami Hiramatsu wrote:
> On Thu, 23 Nov 2023 11:04:00 +0100
> > On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> > > BTW, we also need to make a block-list for some early params. some of those
> > > MUST be passed from the bootloader. E.g. initrd address and size will be
> > > passed from the bootloader via commandline. Thus such params in the embedded
> > > bootconfig should be filtered at the build time.
> > 
> > It's ok to configure these in the embedded bootconfig - in a case they are
> > provided by the bootloader, the bootconfig value is overridden, if not, the
> > value from bootconfig is used, so it works as expected.
> 
> I meant some params only bootloader knows, like where the initrd is loaded.
> Anyway, if user sets such value, it will break the kernel boot as expected :P.

If somebody sets these by hand, he probably knows what is he doing. I
remember myself hardcoding initrd size in Linux to workaround broken
bootloader.

I find it more important to warn the user when he tries to set early
options in initrd (assuming my change gets merged), as that's what
burned me.
  Petr
  

Hi Petr,

On Fri, 24 Nov 2023 02:58:01 +0100
Petr Malat <oss@malat.biz> wrote:

> On Thu, Nov 23, 2023 at 07:41:06PM +0900, Masami Hiramatsu wrote:
> > On Wed, 22 Nov 2023 00:13:42 +0100
> > Petr Malat <oss@malat.biz> wrote:
> > > +	static int prev_rtn __initdata;
> > 
> > I would rather like 'done_retval' instead of 'prev_rtn'.
>  
> OK, I will rename it.
> 
> > > -	/* Cut out the bootconfig data even if we have no bootconfig option */
> > > -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > > -	/* If there is no bootconfig in initrd, try embedded one. */
> > > -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> > > -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > > +	if (prev_rtn)
> > > +		return prev_rtn;
> > >  
> > > +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > >  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> > >  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> > >  			 bootconfig_params);
> > 
> > This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
> > this only checks "bootconfig" is in the cmdline.
> > Can you introduce following flag and initialize it here?
> > 
> > #ifdef CONFIG_BOOT_CONFIG_FORCE
> > #define bootconfig_enabled	(true)
> > #else
> > static bool bootconfig_enabled __initdata;
> > #endif
> 
> Even when CONFIG_BOOT_CONFIG_FORCE is set, we must call parse_args to find
> the location of -- to know where init options should be added. It's done the
> same way in the current code.

Ah, got it.

> 
>  
> > > -
> > > -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> > > -		return;
> > > -
> > > +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> > > +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> > > +		return prev_rtn;
> > 
> > Than we don't need to set a strange error value...
> 
> It's used for error logging as the current code emits a different
> messages in these situations, but I will try to refactor it.

Yeah, I recommend you to set a global flag if initializing something.

> > > +	}
> > >  	/* parse_args() stops at the next param of '--' and returns an address */
> > >  	if (err)
> > >  		initargs_offs = err - tmp_cmdline;
> > >  
> > > -	if (!initrd_data && !embeded_data) {
> > > -		/* If user intended to use bootconfig, show an error level message */
> > > -		if (bootconfig_found)
> > > -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> > > -		else
> > > -			pr_info("No bootconfig data provided, so skipping bootconfig");
> > > -		return;
> > > +	if (!embeded_data) {
> > > +		prev_rtn = -ENOPROTOOPT;
> > 
> > Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
> > using this error code.
> 
> ok, I will try to refactor the error logging. Calling
> xbc_get_embedded_bootconfig is cheap.

Yes.

> 
> > > +		return prev_rtn;
> > >  	}
> > >  
> > >  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> > > -	if (ret < 0)
> > > -		goto err0;
> > > +	if (ret < 0) {
> > > +		boot_config_pr_err(msg, pos, "embedded");
> > > +		prev_rtn = ret;
> > > +		return prev_rtn;
> > > +	}
> > > +	prev_rtn = 1;
> > 
> > This function should be splitted into init_embedded_boot_config() and
> > apply_boot_config_early(). The latter one should not be called twice.
> >
> > > .....
> > > +
> > > +static void __init setup_boot_config(void)
> > > +{
> > > +	const char *msg, *initrd_data;
> > > +	int pos, ret;
> > > +	size_t initrd_size, s;
> > > +
> > > +	/* Cut out the bootconfig data even if we have no bootconfig option */
> > > +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > > +
> > > +	ret = setup_boot_config_early();
> > 
> > Because you should not apply early_params here, you need to call only
> > init_embedded_boot_config() here.
> 
> setup_boot_config_early() must be called from 2 places, because there is
> no guarantee the architecture specific code calls parse_early_param() - it's
> not mandatory. If it's not called by architecture, it's called quite late
> by start_kernel(), later than setup_boot_config().

Right. I meant that you can skip the second one if the first one is
called.

> I want to avoid different behavior on different architectures, so I always
> process early options in the embedded config only, although on some
> architectures even these from initrd could be used, but it could cause
> issues in the future if the architecture would need to switch.

Ah, I got it. There are 2 cases.

- If setup_arch() calls parse_early_param(), the 2nd setup_boot_config_early()
  in the setup_boot_config() will do nothing.

- If setup_arch() does not call parse_early_param(), the 1st
  setup_boot_config_early() in the setup_boot_config() will apply early params
  but the 2nd setup_boot_config_early() in the parse_early_param() will do nothing.
  
OK. And can you write a comment it?

> 
> 
> > > +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {
> > 
> > Also, this can be
> > 	if (!bootconfig_enabled) {
> > 		if (initrd_data || xbc_get_embedded_bootconfig(&s))
> > 
> > > +		pr_info("Bootconfig data present, but handling is disabled\n");
> > > +		return;
> > 
> > 
> > > +	} else if (ret == -ENODATA) {
> > > +		/* Bootconfig disabled and bootconfig data are not present */
> > > +		return;
> > 
> > this can be removed.
> > 
> > > +	} else if (ret == -ENOPROTOOPT) {
> > 
> > This should be
> > 
> > 	} else {
> > 
> > > +		/* Embedded bootconfig not found */
> > > +		if (!initrd_data) {
> > > +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> > > +			return;
> > > +		}
> > > +		ret = xbc_init(NULL, 0, &msg, &pos);
> > > +		if (ret)
> > > +			goto err0;
> > 
> > > +	} else if (ret < 0) {
> > > +		/* Other error, should be logged already */
> > > +		return;
> > 
> > So this is checked at first.
> > 
> > > +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {
> > 
> > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > should be removed.
> 
> I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> compatible change and I didn't want to risk breaking current use cases.
> My change tries to get early options working without affecting how
> other options are handled, but I think appending the config is more
> reasonable behavior and if you do not see it as a problem to not be
> backward compatible here, I will delete the "replace" behavior.

That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
it must skip setting early_params to avoid "hidden setting" from the
embedded bootconfig.

Thank you,

> 
> I will try to refactor the error handling.
>   Petr
  

Hi Masami,
On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:

Shortened the mail as this seems to be the last open point

> > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > should be removed.
> > 
> > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > compatible change and I didn't want to risk breaking current use cases.
> > My change tries to get early options working without affecting how
> > other options are handled, but I think appending the config is more
> > reasonable behavior and if you do not see it as a problem to not be
> > backward compatible here, I will delete the "replace" behavior.
> 
> That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> it must skip setting early_params to avoid "hidden setting" from the
> embedded bootconfig.
 
That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
would disable early options handling even if the user doesn't use initrd at
all, which we do not want.

I suggest logging a KERN_NOTICE message if any early option was applied and
at the same time embedded bootconfig was replaced.
  Petr
  

On Mon, 27 Nov 2023 16:02:22 +0100
Petr Malat <oss@malat.biz> wrote:

> Hi Masami,
> On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:
> 
> Shortened the mail as this seems to be the last open point
> 
> > > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > > should be removed.
> > > 
> > > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > > compatible change and I didn't want to risk breaking current use cases.
> > > My change tries to get early options working without affecting how
> > > other options are handled, but I think appending the config is more
> > > reasonable behavior and if you do not see it as a problem to not be
> > > backward compatible here, I will delete the "replace" behavior.
> > 
> > That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> > it must skip setting early_params to avoid "hidden setting" from the
> > embedded bootconfig.
>  
> That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
> would disable early options handling even if the user doesn't use initrd at
> all, which we do not want.

Yes, so the config name and comment needs to be updated. The problematic point
is that we can not know where there is an initrd (and bootconfig on initrd)
until parsing/applying the early params. And we have to avoid setting "hidden"
parameter.

> 
> I suggest logging a KERN_NOTICE message if any early option was applied and
> at the same time embedded bootconfig was replaced.

No, that's no good because kernel log can be cleared.
Usually user will check /proc/cmdline to check what parameters are set, so at
least it needs to be updated, but also, we need to keep the original cmdline
for make sure the user can reuse it for kexec. To solve this issue without
contradiction, we need to ask user to set BOOT_CONFIG_EMBED_APPEND_INITRD=y
to support early params in bootconfig. (Thus it will be something like
"BOOT_CONFIG_EMBED_EARLY_PARAM")

Thank you,

>   Petr
  

Hi!
Sorry for late reply, I didn't have much time to work on this the last week.

On Tue, Nov 28, 2023 at 07:13:55AM +0900, Masami Hiramatsu wrote:
> On Mon, 27 Nov 2023 16:02:22 +0100
> Petr Malat <oss@malat.biz> wrote:
> 
> > Hi Masami,
> > On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:
> > 
> > Shortened the mail as this seems to be the last open point
> > 
> > > > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > > > should be removed.
> > > > 
> > > > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > > > compatible change and I didn't want to risk breaking current use cases.
> > > > My change tries to get early options working without affecting how
> > > > other options are handled, but I think appending the config is more
> > > > reasonable behavior and if you do not see it as a problem to not be
> > > > backward compatible here, I will delete the "replace" behavior.
> > > 
> > > That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> > > it must skip setting early_params to avoid "hidden setting" from the
> > > embedded bootconfig.
> >  
> > That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
> > would disable early options handling even if the user doesn't use initrd at
> > all, which we do not want.
> 
> Yes, so the config name and comment needs to be updated. The problematic point
> is that we can not know where there is an initrd (and bootconfig on initrd)
> until parsing/applying the early params. And we have to avoid setting "hidden"
> parameter.
> 
> > 
> > I suggest logging a KERN_NOTICE message if any early option was applied and
> > at the same time embedded bootconfig was replaced.
> 
> No, that's no good because kernel log can be cleared.
> Usually user will check /proc/cmdline to check what parameters are set, so at
> least it needs to be updated, but also, we need to keep the original cmdline
> for make sure the user can reuse it for kexec. To solve this issue without
> contradiction, we need to ask user to set BOOT_CONFIG_EMBED_APPEND_INITRD=y
> to support early params in bootconfig. (Thus it will be something like
> "BOOT_CONFIG_EMBED_EARLY_PARAM")

I made a bit of prototyping and I can add applied early options from embedded
config to /proc/cmdline even when the embedded config is replaced. I don't like
applying these options conditionally, because that leads to confusing behavior
of seeing these options in /proc/cmdline and /proc/bootconfig without actually
applying them. I find that worst than the opposite behavior as if user sets
options in the config and then sees them in both cmdline and bootconfig, he
assumes they have been applied.

We can also introduce BOOT_CONFIG_EMBED_REPLACE_INITRD for backward compatibility
and write that this option will be removed in the future. If nobody complains
for a while, we drop it. Then we are left with the appending only.
  Petr