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

[Marco Felsch <m.felsch@pengutronix.de>]

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?

> The helper lives outside efi/payload and efi/loader, as the variable is
> meant to be used identically when barebox runs as EFI payload and when it
> runs as EFI loader. Executing an application directly from the shell keeps
> passing only the shell arguments and the legacy x86 handover protocol is
> unaffected, because it bypasses the EFI stub that would interpret the load
> options.
> 
> Assisted-by: Claude:fable-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/boot.c          |  1 +
>  efi/Makefile           |  2 +-
>  efi/bootargs.c         | 65 ++++++++++++++++++++++++++++++++++++++++++
>  efi/loader/bootm.c     | 27 ++++++++++++------
>  efi/payload/bootm.c    | 23 ++++++++++-----
>  include/efi/bootargs.h |  9 ++++++
>  test/py/test_shell.py  | 26 +++++++++++++++++
>  7 files changed, 136 insertions(+), 17 deletions(-)
>  create mode 100644 efi/bootargs.c
>  create mode 100644 include/efi/bootargs.h
> 
> diff --git a/common/boot.c b/common/boot.c
> index 39c7c79b2cce..f0c6976b1921 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -210,6 +210,7 @@ int boot_entry(struct bootentry *be, int verbose, int dryrun)
>  		pr_err("Booting entry '%s' failed: %pe\n", be->title, ERR_PTR(ret));
>  
>  	globalvar_set_match("linux.bootargs.dyn.", "");
> +	globalvar_set_match("efi.bootargs.dyn.", "");
>  
>  	return ret;
>  }
> diff --git a/efi/Makefile b/efi/Makefile
> index 7693085c7141..f330c1416dbf 100644
> --- a/efi/Makefile
> +++ b/efi/Makefile
> @@ -6,4 +6,4 @@ obj-$(CONFIG_EFI_RUNTIME)	+= runtime/
>  obj-$(CONFIG_EFI_GUID)		+= guid.o
>  obj-$(CONFIG_EFI_DEVICEPATH)	+= devicepath.o
>  obj-$(CONFIG_EFI)		+= errno.o handle.o efivar.o efivar-filename.o
> -obj-$(CONFIG_EFI)		+= initrd.o
> +obj-$(CONFIG_EFI)		+= initrd.o bootargs.o
> diff --git a/efi/bootargs.c b/efi/bootargs.c
> new file mode 100644
> index 000000000000..4a238388ef3f
> --- /dev/null
> +++ b/efi/bootargs.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * bootargs.c - load options for EFI applications started via bootm
> + *
> + * Copyright (C) 2026 Ahmad Fatoum <a.fatoum@pengutronix.de>
> + */
> +#include <common.h>
> +#include <init.h>
> +#include <malloc.h>
> +#include <magicvar.h>
> +#include <globalvar.h>
> +#include <bootargs.h>
> +#include <efi/bootargs.h>
> +
> +/**
> + * 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()?

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.

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
> 
>