[PATCH v2 00/10] efi: payload: allow passing arguments to UKIs

[PATCH v2 00/10] efi: payload: allow passing arguments to UKIs

[Ahmad Fatoum]

UKIs look like a kernel image, but barebox doesn't detect them as one,
which has barebox treat them as it would treat e.g. an EFI shell: An
executable it can start, but can't pass arguments to and that it assumes
will return to it when it exits.

This doesn't hold of course as a UKI should never return execution to
barebox.

Additionally, to select a UKI profile, barebox will need to add an @N
(where N identifies the profile by number) as the very first load
option.

This is also enabled by this series.

v1 -> v2:
  - New patch "commands: devlookup: honour global.bootm.root_param for -k",
    so scripts can build the root option that bootm would append, e.g. for
    UKIs (Fabian)
  - "common: bootargs: don't leave linux_bootargs dangling after free":
    move from 9/9 to the front of the series, as skipping empty variables
    in globalvar_get_match() made the double free reachable after any boot
    entry
  - "efi: payload: honour bootm dryrun in the EFI application handler":
    move before "always shutdown barebox when booting", so bootm -d no
    longer ends the barebox session at any point in the series; drop the
    commit message sentence that only made sense in the old order
  - "Documentation: efi: describe load options handling": turn the UKI
    note into a paragraph explaining that the Linux bootargs, including
    bootm.appendroot's root=, aren't passed, how systemd-stub treats load
    options with and without Secure Boot, and how to pass root= via
    devlookup -k and global.efi.bootargs.root (Fabian)
  - Rebase onto current next
  - Link to v1: https://lore.barebox.org/barebox/20260826121956.2936414-1-a.fatoum@pengutronix.de/


Ahmad Fatoum (10):
  commands: devlookup: honour global.bootm.root_param for -k
  common: bootargs: don't leave linux_bootargs dangling after free
  common: bootargs: drop legacy bootargs fallback with FLEXIBLE_BOOTARGS
  globalvar: skip empty variables in globalvar_get_match()
  efi: payload: honour bootm dryrun in the EFI application handler
  efi: payload: always shutdown barebox when booting
  efi: payload: pass shell arguments as load options to executed images
  efi: add global.efi.bootargs for bootm'd EFI applications
  Documentation: efi: describe load options handling
  test: py: efiloader: check global.efi.bootargs reaches the kernel

 Documentation/boards/efi.rst                  | 64 +++++++++++++++-
 .../migration-guides/migration-master.rst     | 16 ++++
 Documentation/user/booting-linux.rst          | 12 ++-
 commands/devlookup.c                          |  5 +-
 common/block.c                                |  5 +-
 common/boot.c                                 |  1 +
 common/bootargs.c                             | 15 ++--
 common/globalvar.c                            |  6 +-
 efi/Makefile                                  |  2 +-
 efi/bootargs.c                                | 65 ++++++++++++++++
 efi/loader/bootm.c                            | 27 ++++---
 efi/payload/bootm.c                           | 23 +++++-
 efi/payload/image.c                           | 76 ++++++++++++++-----
 efi/payload/image.h                           |  3 +-
 include/block.h                               |  6 +-
 include/efi/bootargs.h                        |  9 +++
 test/py/test_linux_efiloader.py               |  6 ++
 test/py/test_shell.py                         | 26 +++++++
 18 files changed, 315 insertions(+), 52 deletions(-)
 create mode 100644 efi/bootargs.c
 create mode 100644 include/efi/bootargs.h

-- 
2.47.3

[PATCH v2 01/10] commands: devlookup: honour global.bootm.root_param for -k

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

devlookup -k prints the root= option that bootm would add for
global.bootm.root_dev, but it always calls it root=, even though
global.bootm.root_param now allows bootm to use e.g. verity= instead, so
only the initramfs and not the kernel picks up the root file system.

Let's use the same option name as bootm, so scripts can assemble the
option bootm would append for images it doesn't append it to, like
unified kernel images.

Assisted-by: Claude:opus-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 commands/devlookup.c | 5 +++--
 common/block.c       | 5 +++--
 include/block.h      | 6 ++++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/commands/devlookup.c b/commands/devlookup.c
index 8f3468735267..164c005236de 100644
--- a/commands/devlookup.c
+++ b/commands/devlookup.c
@@ -85,7 +85,8 @@ static int do_devlookup(int argc, char *argv[])
 	}
 
 	if (kernelopt)
-		val = buf = cdev_get_linux_rootarg(cdev);
+		val = buf = cdev_get_linux_rootarg(cdev,
+			getenv_nonempty("global.bootm.root_param") ?: "root");
 	else if (paramname)
 		val = dev_get_param(cdev->dev, paramname);
 	else
@@ -109,7 +110,7 @@ BAREBOX_CMD_HELP_TEXT("parameter with that name is looked up.")
 BAREBOX_CMD_HELP_TEXT("")
 BAREBOX_CMD_HELP_TEXT("Options:")
 BAREBOX_CMD_HELP_OPT ("-v",  "write output to VARIABLE instead of printing it")
-BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line")
+BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line (see global.bootm.root_param)")
 BAREBOX_CMD_HELP_OPT ("-a",  "output resolution of cdev alias")
 BAREBOX_CMD_HELP_END
 
diff --git a/common/block.c b/common/block.c
index 5441c7ca1848..16075bb146ec 100644
--- a/common/block.c
+++ b/common/block.c
@@ -665,7 +665,8 @@ int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
 	return 0;
 }
 
-char *cdev_get_linux_rootarg(const struct cdev *partcdev)
+char *cdev_get_linux_rootarg(const struct cdev *partcdev,
+			     const char *root_param)
 {
 	int err;
 	const char *root = NULL;
@@ -675,5 +676,5 @@ char *cdev_get_linux_rootarg(const struct cdev *partcdev)
 	if (err)
 		return NULL;
 
-	return format_root_bootarg("root", root, rootopts);
+	return format_root_bootarg(root_param, root, rootopts);
 }
diff --git a/include/block.h b/include/block.h
index 065ae81358b4..91ab91f9109b 100644
--- a/include/block.h
+++ b/include/block.h
@@ -109,7 +109,8 @@ static inline int block_flush(struct block_device *blk)
 
 #ifdef CONFIG_BLOCK
 unsigned file_list_add_blockdevs(struct file_list *files);
-char *cdev_get_linux_rootarg(const struct cdev *partcdev);
+char *cdev_get_linux_rootarg(const struct cdev *partcdev,
+			     const char *root_param);
 int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
 				 const char **rootopts);
 #else
@@ -117,7 +118,8 @@ static inline unsigned file_list_add_blockdevs(struct file_list *files)
 {
 	return 0;
 }
-static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev)
+static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev,
+					   const char *root_param)
 {
 	return NULL;
 }
-- 
2.47.3

[PATCH v2 02/10] common: bootargs: don't leave linux_bootargs dangling after free

[Ahmad Fatoum]

linux_bootargs_get() frees the cached command line before recomputing it,
but the early return taken when there are no bootargs leaves the static
pointer referencing the freed allocation, so the next call frees it a
second time.

Clear the pointer right after freeing it.

Fixes: ee4cab9e5874 ("booting: more flexible Linux bootargs generation")
Assisted-by: Claude:opus-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/bootargs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/bootargs.c b/common/bootargs.c
index 36528b8b5827..6373d9ccb1f7 100644
--- a/common/bootargs.c
+++ b/common/bootargs.c
@@ -32,6 +32,7 @@ const char *linux_bootargs_get(void)
 		return linux_bootargs;
 
 	free(linux_bootargs);
+	linux_bootargs = NULL;
 
 	bootargs = globalvar_get_match("linux.bootargs.", " ");
 	if (!strlen(bootargs)) {
-- 
2.47.3

[PATCH v2 03/10] common: bootargs: drop legacy bootargs fallback with FLEXIBLE_BOOTARGS

[Ahmad Fatoum]

linux_bootargs_get() falls back to the legacy bootargs environment variable
whenever the concatenation of all global.linux.bootargs.* variables comes
out empty. That contradicts the documentation, which describes bootargs as
the CONFIG_FLEXIBLE_BOOTARGS=n way of passing a command line, and it only
worked until the first boot entry had run: bootscript_boot() registers
global.linux.bootargs.dyn.ip and .dyn.root, and the separator between the
two empty variables made the result non-empty, so the kernel got a command
line consisting of spaces instead.

Drop the fallback and return NULL when there is nothing to pass. All
callers already handle a NULL command line, as the fallback could return
NULL as well.

Assisted-by: Claude:opus-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 .../migration-guides/migration-master.rst        | 16 ++++++++++++++++
 Documentation/user/booting-linux.rst             |  6 ++++--
 common/bootargs.c                                | 14 ++++++--------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/Documentation/migration-guides/migration-master.rst b/Documentation/migration-guides/migration-master.rst
index 43d0b27bb0fe..815d1732cee4 100644
--- a/Documentation/migration-guides/migration-master.rst
+++ b/Documentation/migration-guides/migration-master.rst
@@ -27,3 +27,19 @@ Deep probe is now the default on STM32MP13 and STM32MP15
 barebox now enables deep probe for every ``st,stm32mp1xx`` compatible, even
 over ``barebox,disable-deep-probe``. Report to the mailing list if that
 breaks your board.
+
+Legacy bootargs variable ignored with CONFIG_FLEXIBLE_BOOTARGS
+--------------------------------------------------------------
+
+With ``CONFIG_FLEXIBLE_BOOTARGS`` enabled, the kernel command line used to
+fall back to the legacy ``bootargs`` environment variable whenever the
+concatenation of all ``global.linux.bootargs.*`` variables came out empty.
+This fallback is gone, only the global variables are used now.
+
+The fallback was already mostly unreachable: once a boot entry had run,
+``global.linux.bootargs.dyn.ip`` and ``global.linux.bootargs.dyn.root`` were
+registered and the separator between the two empty variables made the
+concatenation non-empty.
+
+Set ``global.linux.bootargs.base`` instead of ``bootargs``, or disable
+``CONFIG_FLEXIBLE_BOOTARGS`` to keep using the legacy variable.
diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
index 529d4de5c5b8..1608fc1ea431 100644
--- a/Documentation/user/booting-linux.rst
+++ b/Documentation/user/booting-linux.rst
@@ -86,8 +86,10 @@ The simple method to pass bootargs to the kernel is with
 takes the bootargs from the :ref:`bootargs <magicvar_bootargs>` environment variable.
 
 With ``CONFIG_FLEXIBLE_BOOTARGS`` enabled, the bootargs are composed
-from different :ref:`global device<global_device>` variables. All variables beginning
-with ``global.linux.bootargs.`` will be concatenated to the bootargs:
+from different :ref:`global device<global_device>` variables and the
+:ref:`bootargs <magicvar_bootargs>` environment variable is ignored.
+All variables beginning with ``global.linux.bootargs.`` will be concatenated
+to the bootargs:
 
 .. code-block:: sh
 
diff --git a/common/bootargs.c b/common/bootargs.c
index 6373d9ccb1f7..91e92e594876 100644
--- a/common/bootargs.c
+++ b/common/bootargs.c
@@ -17,12 +17,10 @@ static int linux_bootargs_overwritten;
 /*
  * This returns the Linux bootargs
  *
- * There are two ways to handle bootargs. The old legacy way is to use the
- * 'bootargs' environment variable. The new and more flexible way is to use
- * global variables beginning with "global.linux.bootargs." and
- * "global.linux.mtdparts.". These variables will be concatenated together to
- * the resulting bootargs. If there are no "global.linux.bootargs." variables
- * we fall back to "bootargs"
+ * The bootargs are concatenated from the global variables beginning with
+ * "global.linux.bootargs.", "global.linux.mtdparts." and
+ * "global.linux.blkdevparts.". The legacy 'bootargs' environment variable
+ * is only used by the CONFIG_FLEXIBLE_BOOTARGS=n stub in <bootargs.h>.
  */
 const char *linux_bootargs_get(void)
 {
@@ -35,9 +33,9 @@ const char *linux_bootargs_get(void)
 	linux_bootargs = NULL;
 
 	bootargs = globalvar_get_match("linux.bootargs.", " ");
-	if (!strlen(bootargs)) {
+	if (!*bootargs) {
 		free(bootargs);
-		return getenv("bootargs");
+		return NULL;
 	}
 
 	linux_bootargs = bootargs;
-- 
2.47.3

[PATCH v2 04/10] globalvar: skip empty variables in globalvar_get_match()

[Ahmad Fatoum]

globalvar_get_match() concatenates all matching variables with the
separator, whether they have a value or not. As variables like
global.linux.bootargs.base or global.linux.bootargs.dyn.* are registered
unconditionally, but often left empty, the resulting kernel command line
would contain runs of multiple spaces.

Treat variables without a value as if they didn't exist at all.

Assisted-by: Claude:fable-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/user/booting-linux.rst | 2 +-
 common/globalvar.c                   | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
index 1608fc1ea431..8e6f3bbbe9ce 100644
--- a/Documentation/user/booting-linux.rst
+++ b/Documentation/user/booting-linux.rst
@@ -89,7 +89,7 @@ With ``CONFIG_FLEXIBLE_BOOTARGS`` enabled, the bootargs are composed
 from different :ref:`global device<global_device>` variables and the
 :ref:`bootargs <magicvar_bootargs>` environment variable is ignored.
 All variables beginning with ``global.linux.bootargs.`` will be concatenated
-to the bootargs:
+to the bootargs. Variables without a value are skipped:
 
 .. code-block:: sh
 
diff --git a/common/globalvar.c b/common/globalvar.c
index 876379b2538e..41f3e93b00e1 100644
--- a/common/globalvar.c
+++ b/common/globalvar.c
@@ -7,6 +7,7 @@
 #include <init.h>
 #include <environment.h>
 #include <magicvar.h>
+#include <string.h>
 #include <fs.h>
 #include <fcntl.h>
 #include <libfile.h>
@@ -419,7 +420,8 @@ void globalvar_print(void)
  * globalvar_get_match
  *
  * get a concatenated string of all globalvars beginning with 'match'.
- * This adds whitespaces between the different globalvars
+ * This adds the separator between the different globalvars. Variables
+ * without a value are skipped.
  */
 char *globalvar_get_match(const char *match, const char *separator)
 {
@@ -429,6 +431,8 @@ char *globalvar_get_match(const char *match, const char *separator)
 	dev_for_each_param(&global_device, param) {
 		if (!strncmp(match, param->name, strlen(match))) {
 			const char *p = dev_get_param(&global_device, param->name);
+			if (isempty(p))
+				continue;
 			if (val) {
 				char *new = basprintf("%s%s%s", val,
 							separator, p);
-- 
2.47.3

[PATCH v2 05/10] efi: payload: honour bootm dryrun in the EFI application handler

[Ahmad Fatoum]

Unlike do_bootm_efi_stub(), the "EFI Application" bootm handler never
looked at data->dryrun and went on to StartImage the application, so
"bootm -d some.efi" ran the image it was only supposed to load.

Unload the image and return once it has been loaded and its type
detected, which is as far as the stub handler goes for a dryrun too.

Fixes: 35fb1743f5ac ("efi: payload: bootm: add support for efi stub boot")
Assisted-by: Claude:opus-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 efi/payload/bootm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
index 2f9cc3cbf76b..3168e07a3dc1 100644
--- a/efi/payload/bootm.c
+++ b/efi/payload/bootm.c
@@ -220,6 +220,11 @@ static int efi_app_execute(struct image_data *data)
 
 	type = file_detect_type(loaded_image->image_base, PAGE_SIZE);
 
+	if (data->dryrun) {
+		BS->unload_image(handle);
+		return 0;
+	}
+
 	return efi_execute_image(handle, loaded_image, type);
 }
 
-- 
2.47.3

[PATCH v2 06/10] efi: payload: always shutdown barebox when booting

[Ahmad Fatoum]

When barebox is about to boot a kernel image, it calls shutdown_barebox
and then starts the image. When the kernel image is wrapped in a UKI,
barebox will not detect it as a kernel image and will thus not call
shutdown_barebox beforehand, which can mean that e.g. state is not
flushed.

We do not want to treat UKIs completely like kernels (e.g. we do not
want to override their built-in bootargs), but we still want to properly
shutdown barebox.

Resolve this by shutting down barebox whenever we are in a bootm handler,
no matter which kind of image is about to be started: bootm is the point
of no return. Images run from the shell via binfmt keep returning to
barebox afterwards, with the exception of EFI-stubbed kernels, which take
over the machine and thus continue to shut barebox down as before.

Assisted-by: Claude:opus-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 efi/payload/bootm.c |  4 ++--
 efi/payload/image.c | 21 +++++++++++++++------
 efi/payload/image.h |  1 +
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
index 3168e07a3dc1..2bcfd90e42fa 100644
--- a/efi/payload/bootm.c
+++ b/efi/payload/bootm.c
@@ -185,7 +185,7 @@ static int do_bootm_efi_stub(struct image_data *data)
 	if (data->dryrun)
 		goto unload_ramdisk;
 
-	ret = efi_execute_image(handle, loaded_image, type);
+	ret = efi_execute_image(handle, loaded_image, true, type);
 
 	/* efi_execute_image takes care to unload the image on error,
 	 * so we set image_freed and fall through to freeing ramdisk
@@ -225,7 +225,7 @@ static int efi_app_execute(struct image_data *data)
 		return 0;
 	}
 
-	return efi_execute_image(handle, loaded_image, type);
+	return efi_execute_image(handle, loaded_image, true, type);
 }
 
 static int linux_efi_handover = true;
diff --git a/efi/payload/image.c b/efi/payload/image.c
index 378709b6de95..3668a6a04cac 100644
--- a/efi/payload/image.c
+++ b/efi/payload/image.c
@@ -100,15 +100,24 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
 
 int efi_execute_image(efi_handle_t handle,
 		      struct efi_loaded_image *loaded_image,
+		      bool is_bootm,
 		      enum filetype filetype)
 {
 	efi_status_t efiret;
 	const char *options;
-	bool is_driver, is_kernel = false;
+	bool is_driver;
+	bool no_return;
 
 	is_driver = (loaded_image->image_code_type == EFI_BOOT_SERVICES_CODE) ||
 		(loaded_image->image_code_type == EFI_RUNTIME_SERVICES_CODE);
 
+	/*
+	 * A bootm handler is the point of no return, but an EFI-stubbed kernel
+	 * started from the shell takes over the machine just the same, so
+	 * barebox needs to be shut down in both cases.
+	 */
+	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
+
 	if (filetype_is_linux_efi_image(filetype)) {
 		options = linux_bootargs_get();
 		printf("Booting kernel via StartImage");
@@ -119,11 +128,11 @@ int efi_execute_image(efi_handle_t handle,
 				(strlen(options) + 1) * sizeof(wchar_t);
 		}
 		printf("...\n");
+	}
 
+	if (no_return) {
 		efi_set_variable_usec("LoaderTimeExecUSec", &efi_systemd_vendor_guid,
 				      ktime_to_us(ktime_get()));
-
-		is_kernel = true;
 		shutdown_barebox();
 	}
 
@@ -133,8 +142,8 @@ int efi_execute_image(efi_handle_t handle,
 
 	efi_continue_devices();
 
-	if (is_kernel) {
-		pr_emerg("Kernel image has unexpectedly returned\n");
+	if (no_return) {
+		pr_emerg("Boot image has unexpectedly returned\n");
 		BS->exit(efi_parent_image, efiret, 0, NULL);
 	}
 
@@ -160,7 +169,7 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
 	if (ret)
 		return ret;
 
-	return efi_execute_image(handle, loaded_image, b->type);
+	return efi_execute_image(handle, loaded_image, false, b->type);
 }
 
 static struct binfmt_hook binfmt_efi_hook = {
diff --git a/efi/payload/image.h b/efi/payload/image.h
index bab1be368c21..33f7e1a21b30 100644
--- a/efi/payload/image.h
+++ b/efi/payload/image.h
@@ -13,6 +13,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
 
 int efi_execute_image(efi_handle_t handle,
 		      struct efi_loaded_image *loaded_image,
+		      bool is_bootm,
 		      enum filetype filetype);
 
 extern struct image_handler efi_x86_linux_handle_tr;
-- 
2.47.3

[PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Ahmad Fatoum]

EFI applications executed directly from the shell via the binfmt hook,
e.g. "/boot/shell.efi -nostartup", currently have their arguments silently
dropped: the only load options ever set are the Linux bootargs and only if
the image was detected as an EFI-stubbed kernel.

Serialize the arguments following the image path into the load options
instead and leave it to the caller of efi_execute_image() to decide what
the load options should be. bootm keeps passing the Linux bootargs for
kernel images, but executing a kernel image directly from the shell now
passes exactly what was typed on the command line, which makes the
documentation's claim that only bootm passes the kernel command line true
again.

The firmware unloads an application as soon as it returns, as does barebox'
own loader in efi_exit() and EDK2 in CoreStartImage(), so the load options
may only be cleared for a driver that started successfully and is thus
still around. The buffer itself belongs to barebox and is freed either way.

While at it, add the missing space in the "Booting kernel via StartImage
with options" message.

Assisted-by: Claude:fable-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 efi/payload/bootm.c |  9 ++++++--
 efi/payload/image.c | 51 ++++++++++++++++++++++++++++++++++-----------
 efi/payload/image.h |  2 +-
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
index 2bcfd90e42fa..fe2d27b7ff10 100644
--- a/efi/payload/bootm.c
+++ b/efi/payload/bootm.c
@@ -22,6 +22,7 @@
 #include <string.h>
 #include <linux/err.h>
 #include <boot.h>
+#include <bootargs.h>
 #include <bootm.h>
 #include <fs.h>
 #include <libfile.h>
@@ -185,7 +186,9 @@ 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);
+	ret = efi_execute_image(handle, loaded_image, true, type,
+				filetype_is_linux_efi_image(type) ?
+				linux_bootargs_get() : NULL);
 
 	/* efi_execute_image takes care to unload the image on error,
 	 * so we set image_freed and fall through to freeing ramdisk
@@ -225,7 +228,9 @@ static int efi_app_execute(struct image_data *data)
 		return 0;
 	}
 
-	return efi_execute_image(handle, loaded_image, true, type);
+	return efi_execute_image(handle, loaded_image, true, type,
+				 filetype_is_linux_efi_image(type) ?
+				 linux_bootargs_get() : NULL);
 }
 
 static int linux_efi_handover = true;
diff --git a/efi/payload/image.c b/efi/payload/image.c
index 3668a6a04cac..445eaaa587d9 100644
--- a/efi/payload/image.c
+++ b/efi/payload/image.c
@@ -18,7 +18,6 @@
 #include <malloc.h>
 #include <string.h>
 #include <linux/err.h>
-#include <bootargs.h>
 #include <bootm.h>
 #include <fs.h>
 #include <libfile.h>
@@ -101,10 +100,10 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
 int efi_execute_image(efi_handle_t handle,
 		      struct efi_loaded_image *loaded_image,
 		      bool is_bootm,
-		      enum filetype filetype)
+		      enum filetype filetype, const char *options)
 {
 	efi_status_t efiret;
-	const char *options;
+	wchar_t *load_options = NULL;
 	bool is_driver;
 	bool no_return;
 
@@ -118,16 +117,20 @@ int efi_execute_image(efi_handle_t handle,
 	 */
 	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
 
+	if (options && *options) {
+		load_options = xstrdup_char_to_wchar(options);
+		loaded_image->load_options = load_options;
+		loaded_image->load_options_size =
+			(strlen(options) + 1) * sizeof(wchar_t);
+	}
+
 	if (filetype_is_linux_efi_image(filetype)) {
-		options = linux_bootargs_get();
 		printf("Booting kernel via StartImage");
-		if (options) {
-			printf("with options '%s'", options);
-			loaded_image->load_options = xstrdup_char_to_wchar(options);
-			loaded_image->load_options_size =
-				(strlen(options) + 1) * sizeof(wchar_t);
-		}
+		if (load_options)
+			printf(" with options '%s'", options);
 		printf("...\n");
+	} else if (load_options) {
+		pr_debug("Starting image with options '%s'\n", options);
 	}
 
 	if (no_return) {
@@ -150,8 +153,24 @@ int efi_execute_image(efi_handle_t handle,
 	if (EFI_ERROR(efiret))
 		pr_err("failed to StartImage: %s\n", efi_strerror(efiret));
 
-	if (!is_driver)
+	/*
+	 * The firmware unloads an application as soon as it returns, as well as
+	 * a driver that failed to start, freeing the loaded image protocol with
+	 * it. Only a still loaded driver's protocol may be touched here, and it
+	 * must be, as it references the load options we are about to free.
+	 * Unloading an application that already returned just fails, but is
+	 * still needed when StartImage failed before running it.
+	 */
+	if (is_driver) {
+		if (!EFI_ERROR(efiret)) {
+			loaded_image->load_options = NULL;
+			loaded_image->load_options_size = 0;
+		}
+	} else {
 		BS->unload_image(handle);
+	}
+
+	free(load_options);
 
 	efi_connect_all();
 	efi_register_devices();
@@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
 {
 	struct efi_loaded_image *loaded_image;
 	efi_handle_t handle;
+	char *options;
 	int ret;
 
 	ret = efi_load_image(file, &loaded_image, &handle);
 	if (ret)
 		return ret;
 
-	return efi_execute_image(handle, loaded_image, false, b->type);
+	/* argv[0] is the image itself, the rest become the load options */
+	options = strjoin(" ", &argv[1], argc - 1);
+
+	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
+
+	free(options);
+
+	return ret;
 }
 
 static struct binfmt_hook binfmt_efi_hook = {
diff --git a/efi/payload/image.h b/efi/payload/image.h
index 33f7e1a21b30..54494c71626a 100644
--- a/efi/payload/image.h
+++ b/efi/payload/image.h
@@ -14,7 +14,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
 int efi_execute_image(efi_handle_t handle,
 		      struct efi_loaded_image *loaded_image,
 		      bool is_bootm,
-		      enum filetype filetype);
+		      enum filetype filetype, const char *options);
 
 extern struct image_handler efi_x86_linux_handle_tr;
 extern struct image_handler efi_x86_linux_handle_handover;
-- 
2.47.3

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

[Ahmad Fatoum]

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

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) {
+		free(efi_args);
+		efi_args = NULL;
+	}
+
+	if (linux_image)
+		linux_args = linux_bootargs_get();
+	if (linux_args && !*linux_args)
+		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);
+
+	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

[PATCH v2 09/10] Documentation: efi: describe load options handling

[Ahmad Fatoum]

Document that arguments following an EFI application's path on the shell
command line become its load options, that bootm populates the load options
from global.efi.bootargs.* and that the Linux bootargs follow for
EFI-stubbed kernels, both for barebox as EFI payload and as EFI loader.

Assisted-by: Claude:fable-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/boards/efi.rst         | 64 +++++++++++++++++++++++++++-
 Documentation/user/booting-linux.rst |  6 ++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/Documentation/boards/efi.rst b/Documentation/boards/efi.rst
index 5f7dd60998dc..2dde52bc6665 100644
--- a/Documentation/boards/efi.rst
+++ b/Documentation/boards/efi.rst
@@ -112,8 +112,70 @@ means passing control to the calling instance. Note that currently the :ref:`com
 command will pass the control to the calling instance rather than resetting
 the CPU. This may change in the future.
 
+Arguments following the path on the command line are passed to the
+application as its load options, so an EFI shell can for example be
+started with:
+
+.. code-block:: sh
+
+  barebox:/ /boot/shell.efi -nostartup
+
 Although the Linux Kernel can be directly executed one should use the :ref:`command_bootm`
-command. Only the bootm command passes the Kernel commandline to the Kernel.
+command. Only the bootm command passes the Kernel commandline to the Kernel
+and provides it with an initrd and a device tree.
+
+.. _efi_load_options:
+
+Load options
+^^^^^^^^^^^^
+
+When booting an EFI application via :ref:`command_bootm`, barebox populates
+its load options from the :ref:`global device<global_device>` variables
+beginning with ``global.efi.bootargs.``. Like the Linux bootargs, these are
+concatenated in lexicographical order of their names. For EFI-stubbed Linux
+kernels, the ``global.linux.bootargs.``-prefixed
+:ref:`kernel arguments <booting_linux>` are appended to the load options,
+so that the EFI stub sees both as the kernel command line:
+
+.. code-block:: sh
+
+  global efi.bootargs.base="efi=debug"
+  global linux.bootargs.base="console=ttyS0,115200"
+
+  bootm /boot/vmlinuz.efi
+
+  ...
+
+  Kernel command line: efi=debug console=ttyS0,115200
+
+This works both when barebox runs as EFI payload and when it acts as
+EFI loader for the application. The load options are not used when booting
+x86 kernels via the legacy handover protocol (``global.linux.efi.handover``),
+as the EFI stub, which would interpret them, is bypassed in that case.
+
+Like :ref:`boot entries <boot_entries>` do for the Linux
+bootargs, boot entries should add load options to variables with ``.dyn``
+in their name, e.g. ``global.efi.bootargs.dyn.initrd``. These are cleared
+after each boot entry, so that load options do not leak into subsequently
+booted entries.
+
+Unified kernel images (UKIs) are not detected as Linux kernels, but as
+regular EFI applications. Only ``global.efi.bootargs.*`` is passed to them
+as load options; the Linux bootargs, including the ``root=`` option added
+by ``global.bootm.appendroot``, are not appended. systemd-stub uses
+non-empty load options in place of the command line embedded in the UKI,
+unless Secure Boot is enabled and the UKI has an embedded command line, in
+which case the load options are ignored.
+
+To pass a ``root=`` option to a UKI, have :ref:`command_devlookup`
+generate it for the root partition, the same way ``global.bootm.appendroot``
+would, including the option name from ``global.bootm.root_param``:
+
+.. code-block:: sh
+
+  devlookup -v rootarg -k /dev/disk0.1
+  global efi.bootargs.root="${rootarg}"
+  bootm /boot/EFI/Linux/uki.efi
 
 Drivers
 ^^^^^^^
diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
index 8e6f3bbbe9ce..dc7ac6dc8716 100644
--- a/Documentation/user/booting-linux.rst
+++ b/Documentation/user/booting-linux.rst
@@ -103,6 +103,9 @@ to the bootargs. Variables without a value are skipped:
 
   Kernel command line: ignore_loglevel console=ttyO0,115200 earlyprintk
 
+When booting an EFI-stubbed kernel as EFI application, the
+:ref:`EFI load options <efi_load_options>` precede the Linux bootargs.
+
 .. _bootargs_concat_order:
 
 Concatenation order
@@ -170,7 +173,8 @@ This takes the kernel from ``/mnt/mmc1/zImage`` (which could be an
 boot entries should always add Kernel command line parameters to variables with
 ``.dyn`` in it. These will be cleared before booting different boot entries.
 This is done so that following boot entries do not leak command line
-parameters from the previous boot entries.
+parameters from the previous boot entries. The same applies to the
+:ref:`EFI load options <efi_load_options>` in ``global.efi.bootargs.dyn.*``.
 
 This entry can be booted with ``boot mmc``. It can also be made the default by
 setting the :ref:`global.boot.default <magicvar_global_boot_default>` variable
-- 
2.47.3

[PATCH v2 10/10] test: py: efiloader: check global.efi.bootargs reaches the kernel

[Ahmad Fatoum]

Set a global.efi.bootargs.* variable before booting the Debian kernel and
verify that it shows up in /proc/cmdline ahead of the Linux bootargs when
booting via the EFI stub and that it is absent when booting the same kernel
without EFI.

Assisted-by: Claude:fable-5
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 test/py/test_linux_efiloader.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/test/py/test_linux_efiloader.py b/test/py/test_linux_efiloader.py
index eae6703e9d49..a39c6804f61a 100644
--- a/test/py/test_linux_efiloader.py
+++ b/test/py/test_linux_efiloader.py
@@ -50,6 +50,8 @@ def configure_bootm(strategy, barebox):
     barebox.run_check(f"global.bootm.image={kernel_path}")
     # Speed up subsequent runs a bit
     barebox.run_check("global linux.bootargs.noapparmor=apparmor=0")
+    # EFI load options are only used with the EFI stub
+    barebox.run_check("global efi.bootargs.test=efibootargs=1")
 
 
 @pytest.mark.lg_feature(['bootable', 'efi', 'testfs'])
@@ -76,6 +78,8 @@ def test_boot_manual_with_initrd(strategy, barebox, env, efiloader, debian_iso):
                                    "\n".join(dmesg)) is not None
 
         if efiloader:
+            # global.efi.bootargs.* must precede global.linux.bootargs.*
+            shell.run_check("grep -q 'efibootargs=1.* apparmor=0' /proc/cmdline")
             check_efi_kernel_no_warn(shell)
             check_expected_efi_messages(shell, env)
             check_efi_systab(shell, env)
@@ -87,6 +91,8 @@ def test_boot_manual_with_initrd(strategy, barebox, env, efiloader, debian_iso):
             # Verify that EFI was NOT used
             assert uefi_not_found, \
                    "EFI stub was used despite global.bootm.efi=disabled"
+            _, _, ret = shell.run("grep -q efibootargs=1 /proc/cmdline")
+            assert ret != 0, "EFI load options passed despite non-EFI boot"
 
 
 def check_efi_kernel_no_warn(shell):
-- 
2.47.3

Re: [PATCH v2 01/10] commands: devlookup: honour global.bootm.root_param for -k

[Marco Felsch]

Hi Ahmad,

On 26-10-15 11:18, Ahmad Fatoum wrote:
> From: Ahmad Fatoum <a.fatoum@barebox.org>
> 
> devlookup -k prints the root= option that bootm would add for
> global.bootm.root_dev, but it always calls it root=, even though
> global.bootm.root_param now allows bootm to use e.g. verity= instead, so
> only the initramfs and not the kernel picks up the root file system.
> 
> Let's use the same option name as bootm, so scripts can assemble the
> option bootm would append for images it doesn't append it to, like
> unified kernel images.
> 
> Assisted-by: Claude:opus-5.5
> Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
> ---
>  commands/devlookup.c | 5 +++--
>  common/block.c       | 5 +++--
>  include/block.h      | 6 ++++--
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/commands/devlookup.c b/commands/devlookup.c
> index 8f3468735267..164c005236de 100644
> --- a/commands/devlookup.c
> +++ b/commands/devlookup.c
> @@ -85,7 +85,8 @@ static int do_devlookup(int argc, char *argv[])
>  	}
>  
>  	if (kernelopt)
> -		val = buf = cdev_get_linux_rootarg(cdev);
> +		val = buf = cdev_get_linux_rootarg(cdev,
> +			getenv_nonempty("global.bootm.root_param") ?: "root");
				^
Wouldn't it make more sense if cdev_get_linux_rootarg() returns the
correct <key>= so it's fixed for all possible cdev_get_linux_rootarg()
users?
In other words put this logic into cdev_get_linux_rootarg() and keep the
API.

Regards,
  Marco

>  	else if (paramname)
>  		val = dev_get_param(cdev->dev, paramname);
>  	else
> @@ -109,7 +110,7 @@ BAREBOX_CMD_HELP_TEXT("parameter with that name is looked up.")
>  BAREBOX_CMD_HELP_TEXT("")
>  BAREBOX_CMD_HELP_TEXT("Options:")
>  BAREBOX_CMD_HELP_OPT ("-v",  "write output to VARIABLE instead of printing it")
> -BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line")
> +BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line (see global.bootm.root_param)")
>  BAREBOX_CMD_HELP_OPT ("-a",  "output resolution of cdev alias")
>  BAREBOX_CMD_HELP_END
>  
> diff --git a/common/block.c b/common/block.c
> index 5441c7ca1848..16075bb146ec 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -665,7 +665,8 @@ int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
>  	return 0;
>  }
>  
> -char *cdev_get_linux_rootarg(const struct cdev *partcdev)
> +char *cdev_get_linux_rootarg(const struct cdev *partcdev,
> +			     const char *root_param)
>  {
>  	int err;
>  	const char *root = NULL;
> @@ -675,5 +676,5 @@ char *cdev_get_linux_rootarg(const struct cdev *partcdev)
>  	if (err)
>  		return NULL;
>  
> -	return format_root_bootarg("root", root, rootopts);
> +	return format_root_bootarg(root_param, root, rootopts);
>  }
> diff --git a/include/block.h b/include/block.h
> index 065ae81358b4..91ab91f9109b 100644
> --- a/include/block.h
> +++ b/include/block.h
> @@ -109,7 +109,8 @@ static inline int block_flush(struct block_device *blk)
>  
>  #ifdef CONFIG_BLOCK
>  unsigned file_list_add_blockdevs(struct file_list *files);
> -char *cdev_get_linux_rootarg(const struct cdev *partcdev);
> +char *cdev_get_linux_rootarg(const struct cdev *partcdev,
> +			     const char *root_param);
>  int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
>  				 const char **rootopts);
>  #else
> @@ -117,7 +118,8 @@ static inline unsigned file_list_add_blockdevs(struct file_list *files)
>  {
>  	return 0;
>  }
> -static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev)
> +static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev,
> +					   const char *root_param)
>  {
>  	return NULL;
>  }
> -- 
> 2.47.3
> 
> 

Re: [PATCH v2 02/10] common: bootargs: don't leave linux_bootargs dangling after free

[Marco Felsch]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> linux_bootargs_get() frees the cached command line before recomputing it,
> but the early return taken when there are no bootargs leaves the static
> pointer referencing the freed allocation, so the next call frees it a
> second time.
> 
> Clear the pointer right after freeing it.
> 
> Fixes: ee4cab9e5874 ("booting: more flexible Linux bootargs generation")
> Assisted-by: Claude:opus-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH v2 01/10] commands: devlookup: honour global.bootm.root_param for -k

[Ahmad Fatoum]

Hello Marco,

On 10/15/26 11:46 AM, Marco Felsch wrote:
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>> From: Ahmad Fatoum <a.fatoum@barebox.org>
>>  	if (kernelopt)
>> -		val = buf = cdev_get_linux_rootarg(cdev);
>> +		val = buf = cdev_get_linux_rootarg(cdev,
>> +			getenv_nonempty("global.bootm.root_param") ?: "root");
> 				^
> Wouldn't it make more sense if cdev_get_linux_rootarg() returns the
> correct <key>= so it's fixed for all possible cdev_get_linux_rootarg()
> users?
> In other words put this logic into cdev_get_linux_rootarg() and keep the
> API.

There are no other users of this API. I can imagine other users (e.g. a
board's custom boot entry provider) wanting to tightly control whether
global.bootm.root_param is used or not.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>  	else if (paramname)
>>  		val = dev_get_param(cdev->dev, paramname);
>>  	else
>> @@ -109,7 +110,7 @@ BAREBOX_CMD_HELP_TEXT("parameter with that name is looked up.")
>>  BAREBOX_CMD_HELP_TEXT("")
>>  BAREBOX_CMD_HELP_TEXT("Options:")
>>  BAREBOX_CMD_HELP_OPT ("-v",  "write output to VARIABLE instead of printing it")
>> -BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line")
>> +BAREBOX_CMD_HELP_OPT ("-k",  "output kernel rootarg line (see global.bootm.root_param)")
>>  BAREBOX_CMD_HELP_OPT ("-a",  "output resolution of cdev alias")
>>  BAREBOX_CMD_HELP_END
>>  
>> diff --git a/common/block.c b/common/block.c
>> index 5441c7ca1848..16075bb146ec 100644
>> --- a/common/block.c
>> +++ b/common/block.c
>> @@ -665,7 +665,8 @@ int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
>>  	return 0;
>>  }
>>  
>> -char *cdev_get_linux_rootarg(const struct cdev *partcdev)
>> +char *cdev_get_linux_rootarg(const struct cdev *partcdev,
>> +			     const char *root_param)
>>  {
>>  	int err;
>>  	const char *root = NULL;
>> @@ -675,5 +676,5 @@ char *cdev_get_linux_rootarg(const struct cdev *partcdev)
>>  	if (err)
>>  		return NULL;
>>  
>> -	return format_root_bootarg("root", root, rootopts);
>> +	return format_root_bootarg(root_param, root, rootopts);
>>  }
>> diff --git a/include/block.h b/include/block.h
>> index 065ae81358b4..91ab91f9109b 100644
>> --- a/include/block.h
>> +++ b/include/block.h
>> @@ -109,7 +109,8 @@ static inline int block_flush(struct block_device *blk)
>>  
>>  #ifdef CONFIG_BLOCK
>>  unsigned file_list_add_blockdevs(struct file_list *files);
>> -char *cdev_get_linux_rootarg(const struct cdev *partcdev);
>> +char *cdev_get_linux_rootarg(const struct cdev *partcdev,
>> +			     const char *root_param);
>>  int cdev_get_linux_root_and_opts(const struct cdev *partcdev, const char **root,
>>  				 const char **rootopts);
>>  #else
>> @@ -117,7 +118,8 @@ static inline unsigned file_list_add_blockdevs(struct file_list *files)
>>  {
>>  	return 0;
>>  }
>> -static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev)
>> +static inline char *cdev_get_linux_rootarg(const struct cdev *partcdev,
>> +					   const char *root_param)
>>  {
>>  	return NULL;
>>  }
>> -- 
>> 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 |

Re: [PATCH v2 03/10] common: bootargs: drop legacy bootargs fallback with FLEXIBLE_BOOTARGS

[Marco Felsch]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> linux_bootargs_get() falls back to the legacy bootargs environment variable
> whenever the concatenation of all global.linux.bootargs.* variables comes
> out empty. That contradicts the documentation, which describes bootargs as
> the CONFIG_FLEXIBLE_BOOTARGS=n way of passing a command line, and it only
> worked until the first boot entry had run: bootscript_boot() registers
> global.linux.bootargs.dyn.ip and .dyn.root, and the separator between the
> two empty variables made the result non-empty, so the kernel got a command
> line consisting of spaces instead.
> 
> Drop the fallback and return NULL when there is nothing to pass. All
> callers already handle a NULL command line, as the fallback could return
> NULL as well.
> 
> Assisted-by: Claude:opus-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH v2 04/10] globalvar: skip empty variables in globalvar_get_match()

[Marco Felsch]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> globalvar_get_match() concatenates all matching variables with the
> separator, whether they have a value or not. As variables like
> global.linux.bootargs.base or global.linux.bootargs.dyn.* are registered
> unconditionally, but often left empty, the resulting kernel command line
> would contain runs of multiple spaces.
> 
> Treat variables without a value as if they didn't exist at all.
> 
> Assisted-by: Claude:fable-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH v2 06/10] efi: payload: always shutdown barebox when booting

[Marco Felsch]

Hi Ahmad,

On 26-10-15 11:18, Ahmad Fatoum wrote:
> When barebox is about to boot a kernel image, it calls shutdown_barebox
> and then starts the image. When the kernel image is wrapped in a UKI,
> barebox will not detect it as a kernel image and will thus not call
> shutdown_barebox beforehand, which can mean that e.g. state is not
> flushed.
> 
> We do not want to treat UKIs completely like kernels (e.g. we do not
> want to override their built-in bootargs), but we still want to properly
> shutdown barebox.

But it's still possible to pass further kernel commandline arguments to
an UKI?

Regards,
  Marco

> 
> Resolve this by shutting down barebox whenever we are in a bootm handler,
> no matter which kind of image is about to be started: bootm is the point
> of no return. Images run from the shell via binfmt keep returning to
> barebox afterwards, with the exception of EFI-stubbed kernels, which take
> over the machine and thus continue to shut barebox down as before.
> 
> Assisted-by: Claude:opus-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  efi/payload/bootm.c |  4 ++--
>  efi/payload/image.c | 21 +++++++++++++++------
>  efi/payload/image.h |  1 +
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
> index 3168e07a3dc1..2bcfd90e42fa 100644
> --- a/efi/payload/bootm.c
> +++ b/efi/payload/bootm.c
> @@ -185,7 +185,7 @@ static int do_bootm_efi_stub(struct image_data *data)
>  	if (data->dryrun)
>  		goto unload_ramdisk;
>  
> -	ret = efi_execute_image(handle, loaded_image, type);
> +	ret = efi_execute_image(handle, loaded_image, true, type);
>  
>  	/* efi_execute_image takes care to unload the image on error,
>  	 * so we set image_freed and fall through to freeing ramdisk
> @@ -225,7 +225,7 @@ static int efi_app_execute(struct image_data *data)
>  		return 0;
>  	}
>  
> -	return efi_execute_image(handle, loaded_image, type);
> +	return efi_execute_image(handle, loaded_image, true, type);
>  }
>  
>  static int linux_efi_handover = true;
> diff --git a/efi/payload/image.c b/efi/payload/image.c
> index 378709b6de95..3668a6a04cac 100644
> --- a/efi/payload/image.c
> +++ b/efi/payload/image.c
> @@ -100,15 +100,24 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
> +		      bool is_bootm,
>  		      enum filetype filetype)
>  {
>  	efi_status_t efiret;
>  	const char *options;
> -	bool is_driver, is_kernel = false;
> +	bool is_driver;
> +	bool no_return;
>  
>  	is_driver = (loaded_image->image_code_type == EFI_BOOT_SERVICES_CODE) ||
>  		(loaded_image->image_code_type == EFI_RUNTIME_SERVICES_CODE);
>  
> +	/*
> +	 * A bootm handler is the point of no return, but an EFI-stubbed kernel
> +	 * started from the shell takes over the machine just the same, so
> +	 * barebox needs to be shut down in both cases.
> +	 */
> +	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
> +
>  	if (filetype_is_linux_efi_image(filetype)) {
>  		options = linux_bootargs_get();
>  		printf("Booting kernel via StartImage");
> @@ -119,11 +128,11 @@ int efi_execute_image(efi_handle_t handle,
>  				(strlen(options) + 1) * sizeof(wchar_t);
>  		}
>  		printf("...\n");
> +	}
>  
> +	if (no_return) {
>  		efi_set_variable_usec("LoaderTimeExecUSec", &efi_systemd_vendor_guid,
>  				      ktime_to_us(ktime_get()));
> -
> -		is_kernel = true;
>  		shutdown_barebox();
>  	}
>  
> @@ -133,8 +142,8 @@ int efi_execute_image(efi_handle_t handle,
>  
>  	efi_continue_devices();
>  
> -	if (is_kernel) {
> -		pr_emerg("Kernel image has unexpectedly returned\n");
> +	if (no_return) {
> +		pr_emerg("Boot image has unexpectedly returned\n");
>  		BS->exit(efi_parent_image, efiret, 0, NULL);
>  	}
>  
> @@ -160,7 +169,7 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>  	if (ret)
>  		return ret;
>  
> -	return efi_execute_image(handle, loaded_image, b->type);
> +	return efi_execute_image(handle, loaded_image, false, b->type);
>  }
>  
>  static struct binfmt_hook binfmt_efi_hook = {
> diff --git a/efi/payload/image.h b/efi/payload/image.h
> index bab1be368c21..33f7e1a21b30 100644
> --- a/efi/payload/image.h
> +++ b/efi/payload/image.h
> @@ -13,6 +13,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
> +		      bool is_bootm,
>  		      enum filetype filetype);
>  
>  extern struct image_handler efi_x86_linux_handle_tr;
> -- 
> 2.47.3
> 
> 

Re: [PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Marco Felsch]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> EFI applications executed directly from the shell via the binfmt hook,
> e.g. "/boot/shell.efi -nostartup", currently have their arguments silently
> dropped: the only load options ever set are the Linux bootargs and only if
> the image was detected as an EFI-stubbed kernel.
> 
> Serialize the arguments following the image path into the load options
> instead and leave it to the caller of efi_execute_image() to decide what
> the load options should be. bootm keeps passing the Linux bootargs for
> kernel images, but executing a kernel image directly from the shell now
> passes exactly what was typed on the command line, which makes the
> documentation's claim that only bootm passes the kernel command line true
> again.
> 
> The firmware unloads an application as soon as it returns, as does barebox'
> own loader in efi_exit() and EDK2 in CoreStartImage(), so the load options
> may only be cleared for a driver that started successfully and is thus
> still around. The buffer itself belongs to barebox and is freed either way.
> 
> While at it, add the missing space in the "Booting kernel via StartImage
> with options" message.
> 
> Assisted-by: Claude:fable-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  efi/payload/bootm.c |  9 ++++++--
>  efi/payload/image.c | 51 ++++++++++++++++++++++++++++++++++-----------
>  efi/payload/image.h |  2 +-
>  3 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
> index 2bcfd90e42fa..fe2d27b7ff10 100644
> --- a/efi/payload/bootm.c
> +++ b/efi/payload/bootm.c
> @@ -22,6 +22,7 @@
>  #include <string.h>
>  #include <linux/err.h>
>  #include <boot.h>
> +#include <bootargs.h>
>  #include <bootm.h>
>  #include <fs.h>
>  #include <libfile.h>
> @@ -185,7 +186,9 @@ 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);
> +	ret = efi_execute_image(handle, loaded_image, true, type,
> +				filetype_is_linux_efi_image(type) ?
> +				linux_bootargs_get() : NULL);
>  
>  	/* efi_execute_image takes care to unload the image on error,
>  	 * so we set image_freed and fall through to freeing ramdisk
> @@ -225,7 +228,9 @@ static int efi_app_execute(struct image_data *data)
>  		return 0;
>  	}
>  
> -	return efi_execute_image(handle, loaded_image, true, type);
> +	return efi_execute_image(handle, loaded_image, true, type,
> +				 filetype_is_linux_efi_image(type) ?
> +				 linux_bootargs_get() : NULL);
>  }
>  
>  static int linux_efi_handover = true;
> diff --git a/efi/payload/image.c b/efi/payload/image.c
> index 3668a6a04cac..445eaaa587d9 100644
> --- a/efi/payload/image.c
> +++ b/efi/payload/image.c
> @@ -18,7 +18,6 @@
>  #include <malloc.h>
>  #include <string.h>
>  #include <linux/err.h>
> -#include <bootargs.h>
>  #include <bootm.h>
>  #include <fs.h>
>  #include <libfile.h>
> @@ -101,10 +100,10 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
>  		      bool is_bootm,
> -		      enum filetype filetype)
> +		      enum filetype filetype, const char *options)
>  {
>  	efi_status_t efiret;
> -	const char *options;
> +	wchar_t *load_options = NULL;
>  	bool is_driver;
>  	bool no_return;
>  
> @@ -118,16 +117,20 @@ int efi_execute_image(efi_handle_t handle,
>  	 */
>  	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
>  
> +	if (options && *options) {
> +		load_options = xstrdup_char_to_wchar(options);
> +		loaded_image->load_options = load_options;
> +		loaded_image->load_options_size =
> +			(strlen(options) + 1) * sizeof(wchar_t);
> +	}
> +
>  	if (filetype_is_linux_efi_image(filetype)) {
> -		options = linux_bootargs_get();
>  		printf("Booting kernel via StartImage");
> -		if (options) {
> -			printf("with options '%s'", options);
> -			loaded_image->load_options = xstrdup_char_to_wchar(options);
> -			loaded_image->load_options_size =
> -				(strlen(options) + 1) * sizeof(wchar_t);
> -		}
> +		if (load_options)
> +			printf(" with options '%s'", options);
>  		printf("...\n");
> +	} else if (load_options) {
> +		pr_debug("Starting image with options '%s'\n", options);
>  	}

Can we please make use one print style either printf() or pr_*()? IMHO
pr_*() is better since this goes into the log buffer which could be
pstore backed.

Furthermore this if-else now is only responsible for handling the print
message, can we make this cleaner?

>  
>  	if (no_return) {
> @@ -150,8 +153,24 @@ int efi_execute_image(efi_handle_t handle,
>  	if (EFI_ERROR(efiret))
>  		pr_err("failed to StartImage: %s\n", efi_strerror(efiret));
>  
> -	if (!is_driver)
> +	/*
> +	 * The firmware unloads an application as soon as it returns, as well as
> +	 * a driver that failed to start, freeing the loaded image protocol with
> +	 * it. Only a still loaded driver's protocol may be touched here, and it
> +	 * must be, as it references the load options we are about to free.
> +	 * Unloading an application that already returned just fails, but is
> +	 * still needed when StartImage failed before running it.
> +	 */
> +	if (is_driver) {
> +		if (!EFI_ERROR(efiret)) {
> +			loaded_image->load_options = NULL;
					^
Don't we have to free this first?

Regards,
  Marco

> +			loaded_image->load_options_size = 0;
> +		}
> +	} else {
>  		BS->unload_image(handle);
> +	}
> +
> +	free(load_options);
>  
>  	efi_connect_all();
>  	efi_register_devices();
> @@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>  {
>  	struct efi_loaded_image *loaded_image;
>  	efi_handle_t handle;
> +	char *options;
>  	int ret;
>  
>  	ret = efi_load_image(file, &loaded_image, &handle);
>  	if (ret)
>  		return ret;
>  
> -	return efi_execute_image(handle, loaded_image, false, b->type);
> +	/* argv[0] is the image itself, the rest become the load options */
> +	options = strjoin(" ", &argv[1], argc - 1);
> +
> +	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
> +
> +	free(options);
> +
> +	return ret;
>  }
>  
>  static struct binfmt_hook binfmt_efi_hook = {
> diff --git a/efi/payload/image.h b/efi/payload/image.h
> index 33f7e1a21b30..54494c71626a 100644
> --- a/efi/payload/image.h
> +++ b/efi/payload/image.h
> @@ -14,7 +14,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>  int efi_execute_image(efi_handle_t handle,
>  		      struct efi_loaded_image *loaded_image,
>  		      bool is_bootm,
> -		      enum filetype filetype);
> +		      enum filetype filetype, const char *options);
>  
>  extern struct image_handler efi_x86_linux_handle_tr;
>  extern struct image_handler efi_x86_linux_handle_handover;
> -- 
> 2.47.3
> 
> 

Re: [PATCH v2 06/10] efi: payload: always shutdown barebox when booting

[Ahmad Fatoum]

Hi,

On 10/15/26 12:03 PM, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>> When barebox is about to boot a kernel image, it calls shutdown_barebox
>> and then starts the image. When the kernel image is wrapped in a UKI,
>> barebox will not detect it as a kernel image and will thus not call
>> shutdown_barebox beforehand, which can mean that e.g. state is not
>> flushed.
>>
>> We do not want to treat UKIs completely like kernels (e.g. we do not
>> want to override their built-in bootargs), but we still want to properly
>> shutdown barebox.
> 
> But it's still possible to pass further kernel commandline arguments to
> an UKI?

Only by explicit decision via global.efi.bootargs added in a later
commit. Quoting systemd-stub man page:

If UEFI SecureBoot is enabled and the ".cmdline" section is present in
the executed image, any attempts to override the kernel command line by
passing one as invocation parameters to the EFI binary are ignored.
Thus, in order to allow overriding the kernel command line, either
disable UEFI SecureBoot, or do not include a kernel command line PE
section in the kernel image file.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>
>> Resolve this by shutting down barebox whenever we are in a bootm handler,
>> no matter which kind of image is about to be started: bootm is the point
>> of no return. Images run from the shell via binfmt keep returning to
>> barebox afterwards, with the exception of EFI-stubbed kernels, which take
>> over the machine and thus continue to shut barebox down as before.
>>
>> Assisted-by: Claude:opus-5
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  efi/payload/bootm.c |  4 ++--
>>  efi/payload/image.c | 21 +++++++++++++++------
>>  efi/payload/image.h |  1 +
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/efi/payload/bootm.c b/efi/payload/bootm.c
>> index 3168e07a3dc1..2bcfd90e42fa 100644
>> --- a/efi/payload/bootm.c
>> +++ b/efi/payload/bootm.c
>> @@ -185,7 +185,7 @@ static int do_bootm_efi_stub(struct image_data *data)
>>  	if (data->dryrun)
>>  		goto unload_ramdisk;
>>  
>> -	ret = efi_execute_image(handle, loaded_image, type);
>> +	ret = efi_execute_image(handle, loaded_image, true, type);
>>  
>>  	/* efi_execute_image takes care to unload the image on error,
>>  	 * so we set image_freed and fall through to freeing ramdisk
>> @@ -225,7 +225,7 @@ static int efi_app_execute(struct image_data *data)
>>  		return 0;
>>  	}
>>  
>> -	return efi_execute_image(handle, loaded_image, type);
>> +	return efi_execute_image(handle, loaded_image, true, type);
>>  }
>>  
>>  static int linux_efi_handover = true;
>> diff --git a/efi/payload/image.c b/efi/payload/image.c
>> index 378709b6de95..3668a6a04cac 100644
>> --- a/efi/payload/image.c
>> +++ b/efi/payload/image.c
>> @@ -100,15 +100,24 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>>  
>>  int efi_execute_image(efi_handle_t handle,
>>  		      struct efi_loaded_image *loaded_image,
>> +		      bool is_bootm,
>>  		      enum filetype filetype)
>>  {
>>  	efi_status_t efiret;
>>  	const char *options;
>> -	bool is_driver, is_kernel = false;
>> +	bool is_driver;
>> +	bool no_return;
>>  
>>  	is_driver = (loaded_image->image_code_type == EFI_BOOT_SERVICES_CODE) ||
>>  		(loaded_image->image_code_type == EFI_RUNTIME_SERVICES_CODE);
>>  
>> +	/*
>> +	 * A bootm handler is the point of no return, but an EFI-stubbed kernel
>> +	 * started from the shell takes over the machine just the same, so
>> +	 * barebox needs to be shut down in both cases.
>> +	 */
>> +	no_return = is_bootm || filetype_is_linux_efi_image(filetype);
>> +
>>  	if (filetype_is_linux_efi_image(filetype)) {
>>  		options = linux_bootargs_get();
>>  		printf("Booting kernel via StartImage");
>> @@ -119,11 +128,11 @@ int efi_execute_image(efi_handle_t handle,
>>  				(strlen(options) + 1) * sizeof(wchar_t);
>>  		}
>>  		printf("...\n");
>> +	}
>>  
>> +	if (no_return) {
>>  		efi_set_variable_usec("LoaderTimeExecUSec", &efi_systemd_vendor_guid,
>>  				      ktime_to_us(ktime_get()));
>> -
>> -		is_kernel = true;
>>  		shutdown_barebox();
>>  	}
>>  
>> @@ -133,8 +142,8 @@ int efi_execute_image(efi_handle_t handle,
>>  
>>  	efi_continue_devices();
>>  
>> -	if (is_kernel) {
>> -		pr_emerg("Kernel image has unexpectedly returned\n");
>> +	if (no_return) {
>> +		pr_emerg("Boot image has unexpectedly returned\n");
>>  		BS->exit(efi_parent_image, efiret, 0, NULL);
>>  	}
>>  
>> @@ -160,7 +169,7 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>>  	if (ret)
>>  		return ret;
>>  
>> -	return efi_execute_image(handle, loaded_image, b->type);
>> +	return efi_execute_image(handle, loaded_image, false, b->type);
>>  }
>>  
>>  static struct binfmt_hook binfmt_efi_hook = {
>> diff --git a/efi/payload/image.h b/efi/payload/image.h
>> index bab1be368c21..33f7e1a21b30 100644
>> --- a/efi/payload/image.h
>> +++ b/efi/payload/image.h
>> @@ -13,6 +13,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>>  
>>  int efi_execute_image(efi_handle_t handle,
>>  		      struct efi_loaded_image *loaded_image,
>> +		      bool is_bootm,
>>  		      enum filetype filetype);
>>  
>>  extern struct image_handler efi_x86_linux_handle_tr;
>> -- 
>> 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 |

Re: [PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Ahmad Fatoum]

Hello Marco,

On 10/15/26 12:14 PM, Marco Felsch wrote:
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>>  	if (filetype_is_linux_efi_image(filetype)) {
>> -		options = linux_bootargs_get();
>>  		printf("Booting kernel via StartImage");
>> -		if (options) {
>> -			printf("with options '%s'", options);
>> -			loaded_image->load_options = xstrdup_char_to_wchar(options);
>> -			loaded_image->load_options_size =
>> -				(strlen(options) + 1) * sizeof(wchar_t);
>> -		}
>> +		if (load_options)
>> +			printf(" with options '%s'", options);
>>  		printf("...\n");
>> +	} else if (load_options) {
>> +		pr_debug("Starting image with options '%s'\n", options);
>>  	}
> 
> Can we please make use one print style either printf() or pr_*()? IMHO
> pr_*() is better since this goes into the log buffer which could be
> pstore backed.

This is consistent with how we do it for ARM64 and ARM32. An argument in
favor of printf is that test suites often match against these strings as
these are the final strings output by barebox prior to kernel boot.

Having the log level hide them would break this. I don't think it's the
purpose of this series to deviate from this.

> Furthermore this if-else now is only responsible for handling the print
> message, can we make this cleaner?

I don't understand what you mean by this.

>> -	if (!is_driver)
>> +	/*
>> +	 * The firmware unloads an application as soon as it returns, as well as
>> +	 * a driver that failed to start, freeing the loaded image protocol with
>> +	 * it. Only a still loaded driver's protocol may be touched here, and it
>> +	 * must be, as it references the load options we are about to free.
>> +	 * Unloading an application that already returned just fails, but is
>> +	 * still needed when StartImage failed before running it.
>> +	 */
>> +	if (is_driver) {
>> +		if (!EFI_ERROR(efiret)) {
>> +			loaded_image->load_options = NULL;
> 					^
> Don't we have to free this first?

See comment above. This pointer is dangling as the firmware would free
the load options on successful termination.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>> +			loaded_image->load_options_size = 0;
>> +		}
>> +	} else {
>>  		BS->unload_image(handle);
>> +	}
>> +
>> +	free(load_options);
>>  
>>  	efi_connect_all();
>>  	efi_register_devices();
>> @@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
>>  {
>>  	struct efi_loaded_image *loaded_image;
>>  	efi_handle_t handle;
>> +	char *options;
>>  	int ret;
>>  
>>  	ret = efi_load_image(file, &loaded_image, &handle);
>>  	if (ret)
>>  		return ret;
>>  
>> -	return efi_execute_image(handle, loaded_image, false, b->type);
>> +	/* argv[0] is the image itself, the rest become the load options */
>> +	options = strjoin(" ", &argv[1], argc - 1);
>> +
>> +	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
>> +
>> +	free(options);
>> +
>> +	return ret;
>>  }
>>  
>>  static struct binfmt_hook binfmt_efi_hook = {
>> diff --git a/efi/payload/image.h b/efi/payload/image.h
>> index 33f7e1a21b30..54494c71626a 100644
>> --- a/efi/payload/image.h
>> +++ b/efi/payload/image.h
>> @@ -14,7 +14,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
>>  int efi_execute_image(efi_handle_t handle,
>>  		      struct efi_loaded_image *loaded_image,
>>  		      bool is_bootm,
>> -		      enum filetype filetype);
>> +		      enum filetype filetype, const char *options);
>>  
>>  extern struct image_handler efi_x86_linux_handle_tr;
>>  extern struct image_handler efi_x86_linux_handle_handover;
>> -- 
>> 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 |

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

[Marco Felsch]

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

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

[Ahmad Fatoum]

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 |

Re: [PATCH v2 07/10] efi: payload: pass shell arguments as load options to executed images

[Marco Felsch]

On 26-10-15 12:25, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 10/15/26 12:14 PM, Marco Felsch wrote:
> > On 26-10-15 11:18, Ahmad Fatoum wrote:
> >>  	if (filetype_is_linux_efi_image(filetype)) {
> >> -		options = linux_bootargs_get();
> >>  		printf("Booting kernel via StartImage");
> >> -		if (options) {
> >> -			printf("with options '%s'", options);
> >> -			loaded_image->load_options = xstrdup_char_to_wchar(options);
> >> -			loaded_image->load_options_size =
> >> -				(strlen(options) + 1) * sizeof(wchar_t);
> >> -		}
> >> +		if (load_options)
> >> +			printf(" with options '%s'", options);
> >>  		printf("...\n");
> >> +	} else if (load_options) {
> >> +		pr_debug("Starting image with options '%s'\n", options);
> >>  	}
> > 
> > Can we please make use one print style either printf() or pr_*()? IMHO
> > pr_*() is better since this goes into the log buffer which could be
> > pstore backed.
> 
> This is consistent with how we do it for ARM64 and ARM32. An argument in
> favor of printf is that test suites often match against these strings as
> these are the final strings output by barebox prior to kernel boot.
> 
> Having the log level hide them would break this. I don't think it's the
> purpose of this series to deviate from this.
> 
> > Furthermore this if-else now is only responsible for handling the print
> > message, can we make this cleaner?
> 
> I don't understand what you mean by this.

If the strings only differ minimal you could template it, e.g.

     printf("Booting %s", filetype_is_linux_efi_image(fileytpe) ? "kernel via StartImage" : "image")
     if (load_options)
     	printf(" with options '%s'", options)
     printf("...\n");

This harmonizes the output even further by adding '...' not just in the
filetype_is_linux_efi_image() case.

> 
> >> -	if (!is_driver)
> >> +	/*
> >> +	 * The firmware unloads an application as soon as it returns, as well as
> >> +	 * a driver that failed to start, freeing the loaded image protocol with
> >> +	 * it. Only a still loaded driver's protocol may be touched here, and it
> >> +	 * must be, as it references the load options we are about to free.
> >> +	 * Unloading an application that already returned just fails, but is
> >> +	 * still needed when StartImage failed before running it.
> >> +	 */
> >> +	if (is_driver) {
> >> +		if (!EFI_ERROR(efiret)) {
> >> +			loaded_image->load_options = NULL;
> > 					^
> > Don't we have to free this first?
> 
> See comment above. This pointer is dangling as the firmware would free
> the load options on successful termination.

Maybe my EFI knowledge is to limited but the above comments says:

| Only a still loaded driver's protocol may be touched here, and it
| must be, as it references the load options we are about to free.

So we're about the free it but already clear the pointer.

As said, my EFI knowledge may be to limited.

Regards,
  Marco

> 
> Cheers,
> Ahmad
> 
> > 
> > Regards,
> >   Marco
> > 
> >> +			loaded_image->load_options_size = 0;
> >> +		}
> >> +	} else {
> >>  		BS->unload_image(handle);
> >> +	}
> >> +
> >> +	free(load_options);
> >>  
> >>  	efi_connect_all();
> >>  	efi_register_devices();
> >> @@ -163,13 +182,21 @@ static int efi_execute(struct binfmt_hook *b, char *file, int argc, char **argv)
> >>  {
> >>  	struct efi_loaded_image *loaded_image;
> >>  	efi_handle_t handle;
> >> +	char *options;
> >>  	int ret;
> >>  
> >>  	ret = efi_load_image(file, &loaded_image, &handle);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	return efi_execute_image(handle, loaded_image, false, b->type);
> >> +	/* argv[0] is the image itself, the rest become the load options */
> >> +	options = strjoin(" ", &argv[1], argc - 1);
> >> +
> >> +	ret = efi_execute_image(handle, loaded_image, false, b->type, options);
> >> +
> >> +	free(options);
> >> +
> >> +	return ret;
> >>  }
> >>  
> >>  static struct binfmt_hook binfmt_efi_hook = {
> >> diff --git a/efi/payload/image.h b/efi/payload/image.h
> >> index 33f7e1a21b30..54494c71626a 100644
> >> --- a/efi/payload/image.h
> >> +++ b/efi/payload/image.h
> >> @@ -14,7 +14,7 @@ int efi_load_image(const char *file, struct efi_loaded_image **loaded_image,
> >>  int efi_execute_image(efi_handle_t handle,
> >>  		      struct efi_loaded_image *loaded_image,
> >>  		      bool is_bootm,
> >> -		      enum filetype filetype);
> >> +		      enum filetype filetype, const char *options);
> >>  
> >>  extern struct image_handler efi_x86_linux_handle_tr;
> >>  extern struct image_handler efi_x86_linux_handle_handover;
> >> -- 
> >> 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 |
> 

Re: [PATCH v2 09/10] Documentation: efi: describe load options handling

[Marco Felsch]

On 26-10-15 11:18, Ahmad Fatoum wrote:
> Document that arguments following an EFI application's path on the shell
> command line become its load options, that bootm populates the load options
> from global.efi.bootargs.* and that the Linux bootargs follow for
> EFI-stubbed kernels, both for barebox as EFI payload and as EFI loader.
> 
> Assisted-by: Claude:fable-5
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/boards/efi.rst         | 64 +++++++++++++++++++++++++++-
>  Documentation/user/booting-linux.rst |  6 ++-
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/boards/efi.rst b/Documentation/boards/efi.rst
> index 5f7dd60998dc..2dde52bc6665 100644
> --- a/Documentation/boards/efi.rst
> +++ b/Documentation/boards/efi.rst
> @@ -112,8 +112,70 @@ means passing control to the calling instance. Note that currently the :ref:`com
>  command will pass the control to the calling instance rather than resetting
>  the CPU. This may change in the future.
>  
> +Arguments following the path on the command line are passed to the
> +application as its load options, so an EFI shell can for example be
> +started with:
> +
> +.. code-block:: sh
> +
> +  barebox:/ /boot/shell.efi -nostartup
> +
>  Although the Linux Kernel can be directly executed one should use the :ref:`command_bootm`
> -command. Only the bootm command passes the Kernel commandline to the Kernel.
> +command. Only the bootm command passes the Kernel commandline to the Kernel
> +and provides it with an initrd and a device tree.
> +
> +.. _efi_load_options:
> +
> +Load options
> +^^^^^^^^^^^^
> +
> +When booting an EFI application via :ref:`command_bootm`, barebox populates
> +its load options from the :ref:`global device<global_device>` variables
> +beginning with ``global.efi.bootargs.``. Like the Linux bootargs, these are
> +concatenated in lexicographical order of their names. For EFI-stubbed Linux
> +kernels, the ``global.linux.bootargs.``-prefixed
> +:ref:`kernel arguments <booting_linux>` are appended to the load options,
> +so that the EFI stub sees both as the kernel command line:
> +
> +.. code-block:: sh
> +
> +  global efi.bootargs.base="efi=debug"
> +  global linux.bootargs.base="console=ttyS0,115200"
> +
> +  bootm /boot/vmlinuz.efi
> +
> +  ...
> +
> +  Kernel command line: efi=debug console=ttyS0,115200
> +
> +This works both when barebox runs as EFI payload and when it acts as
> +EFI loader for the application. The load options are not used when booting
> +x86 kernels via the legacy handover protocol (``global.linux.efi.handover``),
> +as the EFI stub, which would interpret them, is bypassed in that case.
> +
> +Like :ref:`boot entries <boot_entries>` do for the Linux
> +bootargs, boot entries should add load options to variables with ``.dyn``
> +in their name, e.g. ``global.efi.bootargs.dyn.initrd``. These are cleared
> +after each boot entry, so that load options do not leak into subsequently
> +booted entries.
> +
> +Unified kernel images (UKIs) are not detected as Linux kernels, but as
> +regular EFI applications. Only ``global.efi.bootargs.*`` is passed to them
> +as load options; the Linux bootargs, including the ``root=`` option added
> +by ``global.bootm.appendroot``, are not appended. systemd-stub uses
> +non-empty load options in place of the command line embedded in the UKI,
> +unless Secure Boot is enabled and the UKI has an embedded command line, in
> +which case the load options are ignored.
> +
> +To pass a ``root=`` option to a UKI, have :ref:`command_devlookup`
> +generate it for the root partition, the same way ``global.bootm.appendroot``
> +would, including the option name from ``global.bootm.root_param``:
> +
> +.. code-block:: sh
> +
> +  devlookup -v rootarg -k /dev/disk0.1
> +  global efi.bootargs.root="${rootarg}"
> +  bootm /boot/EFI/Linux/uki.efi

Just a comment on this: devlookup can't be compared to the appendroot
mechansim, due to the partition enumeration mismatch between barebox and
linux, e.g. mmc block device partitions start at 0, whereas linux starts
counting at 1.

On embedded devices booting from an MMC device the root=/dev/mmcblk*
mechanism should be used if the device supports dual boot e.g. from SD
and from eMMC.

Therefore I would have liked to see that the appendroot mechanism would
be supported for UKIs as well which would stop the requirement of having
different UKI profiles just to encode the different boot mediums.

However, as said this is just a comment and doesn't block this patch:

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

>  Drivers
>  ^^^^^^^
> diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
> index 8e6f3bbbe9ce..dc7ac6dc8716 100644
> --- a/Documentation/user/booting-linux.rst
> +++ b/Documentation/user/booting-linux.rst
> @@ -103,6 +103,9 @@ to the bootargs. Variables without a value are skipped:
>  
>    Kernel command line: ignore_loglevel console=ttyO0,115200 earlyprintk
>  
> +When booting an EFI-stubbed kernel as EFI application, the
> +:ref:`EFI load options <efi_load_options>` precede the Linux bootargs.
> +
>  .. _bootargs_concat_order:
>  
>  Concatenation order
> @@ -170,7 +173,8 @@ This takes the kernel from ``/mnt/mmc1/zImage`` (which could be an
>  boot entries should always add Kernel command line parameters to variables with
>  ``.dyn`` in it. These will be cleared before booting different boot entries.
>  This is done so that following boot entries do not leak command line
> -parameters from the previous boot entries.
> +parameters from the previous boot entries. The same applies to the
> +:ref:`EFI load options <efi_load_options>` in ``global.efi.bootargs.dyn.*``.
>  
>  This entry can be booted with ``boot mmc``. It can also be made the default by
>  setting the :ref:`global.boot.default <magicvar_global_boot_default>` variable
> -- 
> 2.47.3
> 
> 

Re: [PATCH v2 09/10] Documentation: efi: describe load options handling

[Ahmad Fatoum]

Hello Marco,

On 10/15/26 1:08 PM, Marco Felsch wrote:
> On 26-10-15 11:18, Ahmad Fatoum wrote:
>> +To pass a ``root=`` option to a UKI, have :ref:`command_devlookup`
>> +generate it for the root partition, the same way ``global.bootm.appendroot``
>> +would, including the option name from ``global.bootm.root_param``:
>> +
>> +.. code-block:: sh
>> +
>> +  devlookup -v rootarg -k /dev/disk0.1
>> +  global efi.bootargs.root="${rootarg}"
>> +  bootm /boot/EFI/Linux/uki.efi
> 
> Just a comment on this: devlookup can't be compared to the appendroot
> mechansim, due to the partition enumeration mismatch between barebox and
> linux, e.g. mmc block device partitions start at 0, whereas linux starts
> counting at 1.
> > On embedded devices booting from an MMC device the root=/dev/mmcblk*
> mechanism should be used if the device supports dual boot e.g. from SD
> and from eMMC.

devlookup -k follows the same code paths appendroot would follow, so it
will be 1-indexed /dev/mmblkXpY if that's what appendroot would have
produced.


> Therefore I would have liked to see that the appendroot mechanism would
> be supported for UKIs as well which would stop the requirement of having
> different UKI profiles just to encode the different boot mediums.

I believe barebox can't provide this universally.

- On an x86 system with secure boot, the systemd-stub will ignore any
extra command-line arguments, except for the profile.

- On the ARM systems with device tree, we may be able to cheat by
patching the DT, but I have not checked how Linux or systemd-boot handle
/chosen/bootargs if they are booted via EFI.

Cheers,
Ahmad


> 
> However, as said this is just a comment and doesn't block this patch:
> 
> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> 
>>  Drivers
>>  ^^^^^^^
>> diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
>> index 8e6f3bbbe9ce..dc7ac6dc8716 100644
>> --- a/Documentation/user/booting-linux.rst
>> +++ b/Documentation/user/booting-linux.rst
>> @@ -103,6 +103,9 @@ to the bootargs. Variables without a value are skipped:
>>  
>>    Kernel command line: ignore_loglevel console=ttyO0,115200 earlyprintk
>>  
>> +When booting an EFI-stubbed kernel as EFI application, the
>> +:ref:`EFI load options <efi_load_options>` precede the Linux bootargs.
>> +
>>  .. _bootargs_concat_order:
>>  
>>  Concatenation order
>> @@ -170,7 +173,8 @@ This takes the kernel from ``/mnt/mmc1/zImage`` (which could be an
>>  boot entries should always add Kernel command line parameters to variables with
>>  ``.dyn`` in it. These will be cleared before booting different boot entries.
>>  This is done so that following boot entries do not leak command line
>> -parameters from the previous boot entries.
>> +parameters from the previous boot entries. The same applies to the
>> +:ref:`EFI load options <efi_load_options>` in ``global.efi.bootargs.dyn.*``.
>>  
>>  This entry can be booted with ``boot mmc``. It can also be made the default by
>>  setting the :ref:`global.boot.default <magicvar_global_boot_default>` variable
>> -- 
>> 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 |

Re: [PATCH v2 09/10] Documentation: efi: describe load options handling

[Marco Felsch]

On 26-10-15 13:19, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 10/15/26 1:08 PM, Marco Felsch wrote:
> > On 26-10-15 11:18, Ahmad Fatoum wrote:
> >> +To pass a ``root=`` option to a UKI, have :ref:`command_devlookup`
> >> +generate it for the root partition, the same way ``global.bootm.appendroot``
> >> +would, including the option name from ``global.bootm.root_param``:
> >> +
> >> +.. code-block:: sh
> >> +
> >> +  devlookup -v rootarg -k /dev/disk0.1
> >> +  global efi.bootargs.root="${rootarg}"
> >> +  bootm /boot/EFI/Linux/uki.efi
> > 
> > Just a comment on this: devlookup can't be compared to the appendroot
> > mechansim, due to the partition enumeration mismatch between barebox and
> > linux, e.g. mmc block device partitions start at 0, whereas linux starts
> > counting at 1.
> > > On embedded devices booting from an MMC device the root=/dev/mmcblk*
> > mechanism should be used if the device supports dual boot e.g. from SD
> > and from eMMC.
> 
> devlookup -k follows the same code paths appendroot would follow, so it
> will be 1-indexed /dev/mmblkXpY if that's what appendroot would have
> produced.

Ah, good to know, thanks.

> > Therefore I would have liked to see that the appendroot mechanism would
> > be supported for UKIs as well which would stop the requirement of having
> > different UKI profiles just to encode the different boot mediums.
> 
> I believe barebox can't provide this universally.
> 
> - On an x86 system with secure boot, the systemd-stub will ignore any
> extra command-line arguments, except for the profile.

According the comment from system-stub, the cmdline is only ignored if
the UKI already has a cmdline and we're in secure-boot:

---8<---------8<---------8<---------8<---------8<---------8<---------8<--
| * We'll suppress the custom cmdline if we are in Secure Boot mode, and if either there is already
| * a cmdline baked into the UKI or we are in confidential VM mode. */ 
---8<---------8<---------8<---------8<---------8<---------8<---------8<--

Regards,
  Marco

> - On the ARM systems with device tree, we may be able to cheat by
> patching the DT, but I have not checked how Linux or systemd-boot handle
> /chosen/bootargs if they are booted via EFI.
> 
> Cheers,
> Ahmad
> 
> 
> > 
> > However, as said this is just a comment and doesn't block this patch:
> > 
> > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> > 
> >>  Drivers
> >>  ^^^^^^^
> >> diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
> >> index 8e6f3bbbe9ce..dc7ac6dc8716 100644
> >> --- a/Documentation/user/booting-linux.rst
> >> +++ b/Documentation/user/booting-linux.rst
> >> @@ -103,6 +103,9 @@ to the bootargs. Variables without a value are skipped:
> >>  
> >>    Kernel command line: ignore_loglevel console=ttyO0,115200 earlyprintk
> >>  
> >> +When booting an EFI-stubbed kernel as EFI application, the
> >> +:ref:`EFI load options <efi_load_options>` precede the Linux bootargs.
> >> +
> >>  .. _bootargs_concat_order:
> >>  
> >>  Concatenation order
> >> @@ -170,7 +173,8 @@ This takes the kernel from ``/mnt/mmc1/zImage`` (which could be an
> >>  boot entries should always add Kernel command line parameters to variables with
> >>  ``.dyn`` in it. These will be cleared before booting different boot entries.
> >>  This is done so that following boot entries do not leak command line
> >> -parameters from the previous boot entries.
> >> +parameters from the previous boot entries. The same applies to the
> >> +:ref:`EFI load options <efi_load_options>` in ``global.efi.bootargs.dyn.*``.
> >>  
> >>  This entry can be booted with ``boot mmc``. It can also be made the default by
> >>  setting the :ref:`global.boot.default <magicvar_global_boot_default>` variable
> >> -- 
> >> 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 |
> 

Re: [PATCH v2 09/10] Documentation: efi: describe load options handling

[Ahmad Fatoum]

Hi,

On 10/15/26 2:13 PM, Marco Felsch wrote:
> On 26-10-15 13:19, Ahmad Fatoum wrote:
>> Hello Marco,
>>
>> On 10/15/26 1:08 PM, Marco Felsch wrote:
>>> On 26-10-15 11:18, Ahmad Fatoum wrote:
>>>> +To pass a ``root=`` option to a UKI, have :ref:`command_devlookup`
>>>> +generate it for the root partition, the same way ``global.bootm.appendroot``
>>>> +would, including the option name from ``global.bootm.root_param``:
>>>> +
>>>> +.. code-block:: sh
>>>> +
>>>> +  devlookup -v rootarg -k /dev/disk0.1
>>>> +  global efi.bootargs.root="${rootarg}"
>>>> +  bootm /boot/EFI/Linux/uki.efi
>>>
>>> Just a comment on this: devlookup can't be compared to the appendroot
>>> mechansim, due to the partition enumeration mismatch between barebox and
>>> linux, e.g. mmc block device partitions start at 0, whereas linux starts
>>> counting at 1.
>>>> On embedded devices booting from an MMC device the root=/dev/mmcblk*
>>> mechanism should be used if the device supports dual boot e.g. from SD
>>> and from eMMC.
>>
>> devlookup -k follows the same code paths appendroot would follow, so it
>> will be 1-indexed /dev/mmblkXpY if that's what appendroot would have
>> produced.
> 
> Ah, good to know, thanks.
> 
>>> Therefore I would have liked to see that the appendroot mechanism would
>>> be supported for UKIs as well which would stop the requirement of having
>>> different UKI profiles just to encode the different boot mediums.
>>
>> I believe barebox can't provide this universally.
>>
>> - On an x86 system with secure boot, the systemd-stub will ignore any
>> extra command-line arguments, except for the profile.
> 
> According the comment from system-stub, the cmdline is only ignored if
> the UKI already has a cmdline and we're in secure-boot:
> 
> ---8<---------8<---------8<---------8<---------8<---------8<---------8<--
> | * We'll suppress the custom cmdline if we are in Secure Boot mode, and if either there is already
> | * a cmdline baked into the UKI or we are in confidential VM mode. */ 
> ---8<---------8<---------8<---------8<---------8<---------8<---------8<--

If you have a good idea how to design the API for this, so it's not
confusing to users, go ahead.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>> - On the ARM systems with device tree, we may be able to cheat by
>> patching the DT, but I have not checked how Linux or systemd-boot handle
>> /chosen/bootargs if they are booted via EFI.
>>
>> Cheers,
>> Ahmad
>>
>>
>>>
>>> However, as said this is just a comment and doesn't block this patch:
>>>
>>> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
>>>
>>>>  Drivers
>>>>  ^^^^^^^
>>>> diff --git a/Documentation/user/booting-linux.rst b/Documentation/user/booting-linux.rst
>>>> index 8e6f3bbbe9ce..dc7ac6dc8716 100644
>>>> --- a/Documentation/user/booting-linux.rst
>>>> +++ b/Documentation/user/booting-linux.rst
>>>> @@ -103,6 +103,9 @@ to the bootargs. Variables without a value are skipped:
>>>>  
>>>>    Kernel command line: ignore_loglevel console=ttyO0,115200 earlyprintk
>>>>  
>>>> +When booting an EFI-stubbed kernel as EFI application, the
>>>> +:ref:`EFI load options <efi_load_options>` precede the Linux bootargs.
>>>> +
>>>>  .. _bootargs_concat_order:
>>>>  
>>>>  Concatenation order
>>>> @@ -170,7 +173,8 @@ This takes the kernel from ``/mnt/mmc1/zImage`` (which could be an
>>>>  boot entries should always add Kernel command line parameters to variables with
>>>>  ``.dyn`` in it. These will be cleared before booting different boot entries.
>>>>  This is done so that following boot entries do not leak command line
>>>> -parameters from the previous boot entries.
>>>> +parameters from the previous boot entries. The same applies to the
>>>> +:ref:`EFI load options <efi_load_options>` in ``global.efi.bootargs.dyn.*``.
>>>>  
>>>>  This entry can be booted with ``boot mmc``. It can also be made the default by
>>>>  setting the :ref:`global.boot.default <magicvar_global_boot_default>` variable
>>>> -- 
>>>> 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 |
>>

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