Re: [PATCH v2 08/10] efi: add global.efi.bootargs for bootm'd EFI applications

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Marco,

On 10/15/26 12:34 PM, Marco Felsch wrote:
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>> The load options of an EFI application booted via bootm can so far only be
>> influenced for EFI-stubbed Linux kernels and only via the Linux bootargs.
>> There is no way to pass options to a generic EFI application like a shell
>> or a boot loader, and no way to pass EFI-stub-specific options like initrd=
>> or efi= to a kernel separately from what goes into the Linux bootargs.
>>
>> Add a global.efi.bootargs.* family of variables that works like
>> global.linux.bootargs.*: all variables with that prefix are concatenated in
>> lexicographical order and become the load options of any EFI application
>> booted via bootm. For EFI-stubbed Linux kernels, the Linux bootargs are
>> appended, so the kernel command line becomes
>> "$global.efi.bootargs $global.linux.bootargs".
> 
> Do we need to document this behavior?

It's documented in the magicvar string, which will be extracted into the
online documentation.

>> +/**
>> + * efi_bootargs_get() - compute load options for an EFI application
>> + * @linux_image: whether the image is an EFI-stubbed Linux kernel
>> + *
>> + * All variables beginning with "global.efi.bootargs." are concatenated in
>> + * lexicographical order. For Linux kernels, the Linux bootargs are appended,
>> + * so the EFI stub sees them as the kernel command line.
>> + *
>> + * Return: newly allocated string to be freed by the caller or NULL
>> + */
>> +char *efi_bootargs_get(bool linux_image)
>> +{
>> +	const char *linux_args = NULL;
>> +	char *efi_args, *options;
>> +
>> +	efi_args = globalvar_get_match("efi.bootargs.", " ");
>> +	if (efi_args && !*efi_args) {
> 		^
> 	isempty()?

isempty would be !efi_args || !*efi_args, which is not equivalent.

> 
> Nit: Would be even nicer to have globalvar_get_match_or_null();
> 
>> +		free(efi_args);
>> +		efi_args = NULL;
>> +	}
>> +
>> +	if (linux_image)
>> +		linux_args = linux_bootargs_get();
>> +	if (linux_args && !*linux_args)
> 		^
> 	isempty()?
> 
>> +		linux_args = NULL;
>> +
>> +	if (efi_args && linux_args)
>> +		options = xasprintf("%s %s", efi_args, linux_args);
>> +	else if (linux_args)
>> +		options = xstrdup(linux_args);
>> +	else
>> +		options = efi_args;
>> +
>> +	if (options != efi_args)
>> +		free(efi_args);
> 
> Albeit I'm a fan of clever code, the handling of the 'options' and
> 'efi_args' here is IMHO overcomplicated and it took me far to long to
> review it.

I think it should be possible to simplify this with strjoin. I can give
it a try when you are done with your review.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>> +
>> +	return options;
>> +}
>> +
>> +static int efi_bootargs_init(void)
>> +{
>> +	globalvar_add_simple("efi.bootargs.base", NULL);
>> +
>> +	return 0;
>> +}
>> +late_initcall(efi_bootargs_init);
>> +
>> +BAREBOX_MAGICVAR(global.efi.bootargs.*,
>> +		 "Load options of EFI applications started via bootm. Linux bootargs are appended for EFI-stubbed kernels");
>> +BAREBOX_MAGICVAR(global.efi.bootargs.dyn.*,
>> +		 "Load options set by boot entries. Cleared after each boot entry");
>> diff --git a/efi/loader/bootm.c b/efi/loader/bootm.c
>> index a68db742e173..e3d654774831 100644
>> --- a/efi/loader/bootm.c
>> +++ b/efi/loader/bootm.c
>> @@ -13,7 +13,6 @@
>>  #include <init.h>
>>  #include <driver.h>
>>  #include <io.h>
>> -#include <bootargs.h>
>>  #include <malloc.h>
>>  #include <string.h>
>>  #include <linux/err.h>
>> @@ -34,6 +33,7 @@
>>  #include <efi/services.h>
>>  #include <efi/error.h>
>>  #include <efi/initrd.h>
>> +#include <efi/bootargs.h>
>>  #include <efi/devicepath.h>
>>  
>>  /**
>> @@ -199,6 +199,7 @@ static int efi_loader_bootm(struct image_data *data)
>>  	resource_size_t start, end;
>>  	void *load_option = NULL;
>>  	u32 load_option_size = 0;
>> +	char *options;
>>  	efi_handle_t handle;
>>  	struct efi_device_path *file_path = NULL;
>>  	struct efi_event *evt;
>> @@ -215,19 +216,19 @@ static int efi_loader_bootm(struct image_data *data)
>>  	if (IS_ERR(os_res))
>>  		return PTR_ERR(os_res);
>>  
>> -	if (filetype_is_linux_efi_image(data->kernel_type)) {
>> -		const char *options;
>> -
>> -		options = linux_bootargs_get();
>> -		if (options) {
>> -			load_option = xstrdup_char_to_wchar(options);
>> -			load_option_size = (strlen(options) + 1) * sizeof(wchar_t);
>> -		}
>> +	options = efi_bootargs_get(filetype_is_linux_efi_image(data->kernel_type));
>> +	if (options) {
>> +		load_option = xstrdup_char_to_wchar(options);
>> +		load_option_size = (strlen(options) + 1) * sizeof(wchar_t);
>>  	}
>>  
>>  	file_path = efi_dp_from_file(AT_FDCWD, data->os_file);
>>  
>>  	pr_info("Loading %pD\n", file_path);
>> +	if (data->verbose && options)
>> +		pr_info("Load options: %s\n", options);
>> +
>> +	free(options);
>>  
>>  	/* Initialize EFI drivers */
>>  	efiret = efi_init_obj_list();
>> @@ -321,6 +322,14 @@ static int efi_loader_bootm(struct image_data *data)
>>  	/* Control is returned to us, disable EFI watchdog */
>>  	efi_set_watchdog(0);
>>  
>> +	/*
>> +	 * A still loaded driver would keep referencing the load options.
>> +	 * efi_set_load_options() tolerates the already deleted handle of an
>> +	 * application.
>> +	 */
>> +	efi_set_load_options(handle, 0, NULL);
>> +	free(load_option);
>> +
>>  	return -efi_errno(efiret);
>>  
>>  out:
>> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
>> index fe2d27b7ff10..491bafb66898 100644
>> --- a/efi/payload/bootm.c
>> +++ b/efi/payload/bootm.c
>> @@ -22,7 +22,6 @@
>>  #include <string.h>
>>  #include <linux/err.h>
>>  #include <boot.h>
>> -#include <bootargs.h>
>>  #include <bootm.h>
>>  #include <fs.h>
>>  #include <libfile.h>
>> @@ -32,6 +31,7 @@
>>  #include <efi/payload/driver.h>
>>  #include <efi/error.h>
>>  #include <efi/initrd.h>
>> +#include <efi/bootargs.h>
>>  
>>  #include "image.h"
>>  
>> @@ -167,6 +167,7 @@ static int do_bootm_efi_stub(struct image_data *data)
>>  	bool image_freed = false;
>>  	efi_handle_t handle = NULL; /* silence compiler warning */
>>  	enum filetype type;
>> +	char *options;
>>  	int ret;
>>  
>>  	ret = efi_load_os(data, &loaded_image, &handle);
>> @@ -186,9 +187,11 @@ static int do_bootm_efi_stub(struct image_data *data)
>>  	if (data->dryrun)
>>  		goto unload_ramdisk;
>>  
>> -	ret = efi_execute_image(handle, loaded_image, true, type,
>> -				filetype_is_linux_efi_image(type) ?
>> -				linux_bootargs_get() : NULL);
>> +	options = efi_bootargs_get(filetype_is_linux_efi_image(type));
>> +
>> +	ret = efi_execute_image(handle, loaded_image, true, type, options);
>> +
>> +	free(options);
>>  
>>  	/* efi_execute_image takes care to unload the image on error,
>>  	 * so we set image_freed and fall through to freeing ramdisk
>> @@ -215,6 +218,7 @@ static int efi_app_execute(struct image_data *data)
>>  	struct efi_loaded_image *loaded_image;
>>  	efi_handle_t handle;
>>  	enum filetype type;
>> +	char *options;
>>  	int ret;
>>  
>>  	ret = efi_load_image(data->os_file, &loaded_image, &handle);
>> @@ -223,14 +227,19 @@ static int efi_app_execute(struct image_data *data)
>>  
>>  	type = file_detect_type(loaded_image->image_base, PAGE_SIZE);
>>  
>> +	options = efi_bootargs_get(filetype_is_linux_efi_image(type));
>> +
>>  	if (data->dryrun) {
>>  		BS->unload_image(handle);
>> +		free(options);
>>  		return 0;
>>  	}
>>  
>> -	return efi_execute_image(handle, loaded_image, true, type,
>> -				 filetype_is_linux_efi_image(type) ?
>> -				 linux_bootargs_get() : NULL);
>> +	ret = efi_execute_image(handle, loaded_image, true, type, options);
>> +
>> +	free(options);
>> +
>> +	return ret;
>>  }
>>  
>>  static int linux_efi_handover = true;
>> diff --git a/include/efi/bootargs.h b/include/efi/bootargs.h
>> new file mode 100644
>> index 000000000000..dfd17261ff9d
>> --- /dev/null
>> +++ b/include/efi/bootargs.h
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef __EFI_BOOTARGS_H
>> +#define __EFI_BOOTARGS_H
>> +
>> +#include <linux/types.h>
>> +
>> +char *efi_bootargs_get(bool linux_image);
>> +
>> +#endif /* __EFI_BOOTARGS_H */
>> diff --git a/test/py/test_shell.py b/test/py/test_shell.py
>> index be746ca205ba..7a3330b16466 100644
>> --- a/test/py/test_shell.py
>> +++ b/test/py/test_shell.py
>> @@ -153,3 +153,29 @@ def test_barebox_test_var_exists(barebox, barebox_config):
>>  
>>      # Clean up
>>      barebox.run_check('rm /tmp/testvars')
>> +
>> +
>> +def test_boot_bootargs_dyn_cleared(barebox, barebox_config):
>> +    skip_disabled(barebox_config, "CONFIG_CMD_BOOT", "CONFIG_CMD_GLOBAL",
>> +                  "CONFIG_CMD_ECHO")
>> +
>> +    barebox.run_check("echo -o /env/boot/dyntest '#!/bin/sh'")
>> +    barebox.run_check("echo -a /env/boot/dyntest "
>> +                      "'global linux.bootargs.dyn.test=linux-dyn'")
>> +    barebox.run_check("echo -a /env/boot/dyntest "
>> +                      "'global efi.bootargs.dyn.test=efi-dyn'")
>> +    barebox.run_check("echo -a /env/boot/dyntest "
>> +                      "'global efi.bootargs.nodyntest=efi-static'")
>> +
>> +    # Run the script, but don't actually boot anything
>> +    barebox.run("boot -d -d dyntest")
>> +
>> +    # .dyn. variables must not leak into subsequent boot entries
>> +    assert barebox.run_check("echo ${global.linux.bootargs.dyn.test}") == [""]
>> +    assert barebox.run_check("echo ${global.efi.bootargs.dyn.test}") == [""]
>> +    # ... but other variables are left alone
>> +    assert barebox.run_check("echo ${global.efi.bootargs.nodyntest}") == \
>> +        ["efi-static"]
>> +
>> +    barebox.run_check("global -r efi.bootargs.nodyntest")
>> +    barebox.run_check("rm /env/boot/dyntest")
>> -- 
>> 2.47.3
>>
>>

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |