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

[Paul E. McKenney]

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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxx>