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 <[email protected]> > --- > 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 <[email protected]> > + */ > +#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 > >
