Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Fri, 24 Nov 2023 02:58:01 +0100
From:   Petr Malat <oss@malat.biz>
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     linux-kernel@vger.kernel.org, paulmck@kernel.org,
        rostedt@goodmis.org
Subject: Re: [PATCH 2/2] bootconfig: Apply early options from embedded config
Message-ID: <ZWADKWURCDZXyJTQ@ntb.petris.klfree.czf>
References: <20231121231342.193646-1-oss@malat.biz>
 <20231121231342.193646-3-oss@malat.biz>
 <20231123194106.08f5832f558fe806b1fd8098@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231123194106.08f5832f558fe806b1fd8098@kernel.org>
Precedence: bulk

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