[PATCH RFC] bootm: support printing bootm parameters determined at runtime to file

[PATCH RFC] bootm: support printing bootm parameters determined at runtime to file

[Ahmad Fatoum]

The barebox bootm command is often not called directly, but via
bootloader spec or FIT image boot handlers. For debugging, it can be
useful to reuse those boot handlers, but replace single artifacts, e.g.
using the barebox device tree instead of the bootloader-spec provided
device tree.

To make this easier, have boot -v -d (verbose + dry run) write a boot
script that reproduces the cancelled boot to /env/boot/cancelled.

The user can then edit the file and boot it manually.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/bootm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 common/bootm.c   | 10 +++++-
 include/bootm.h  |  9 +++++
 3 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/commands/bootm.c b/commands/bootm.c
index 95d267135cdb..520f041b6bac 100644
--- a/commands/bootm.c
+++ b/commands/bootm.c
@@ -8,6 +8,7 @@
 #include <driver.h>
 #include <command.h>
 #include <image.h>
+#include <boot.h>
 #include <malloc.h>
 #include <environment.h>
 #include <asm/byteorder.h>
@@ -36,6 +37,90 @@
 #define BOOTM_OPTS BOOTM_OPTS_COMMON
 #endif
 
+int bootm_print_cmd(const char *file, const struct bootm_data *data)
+{
+	int fd = CONSOLE_STDOUT;
+	const char *bootargs;
+	const char *verify = "";
+
+	if (!IS_ENABLED(CONFIG_GLOBALVAR))
+		return -ENOSYS;
+
+	if (file) {
+		fd = creat(file, 0755);
+		if (fd < 0)
+			return fd;
+	}
+
+	dprintf(fd, "#!/bin/sh\n\n");
+
+	if (data->appendroot)
+		dprintf(fd, "global.bootm.appendroot=%u\n", data->appendroot);
+	if (data->root_dev)
+		dprintf(fd, "global.bootm.root_dev='%s'\n", data->root_dev);
+	if (data->provide_machine_id)
+		dprintf(fd, "global.bootm.machine_id=%u\n", data->provide_machine_id);
+
+	switch (data->verify) {
+	case BOOTM_VERIFY_NONE:
+		verify = "none";
+		break;
+	case BOOTM_VERIFY_HASH:
+		verify = "hash";
+		break;
+	case BOOTM_VERIFY_AVAILABLE:
+		verify = "available";
+		break;
+	case BOOTM_VERIFY_SIGNATURE:
+		verify = "signature";
+		break;
+	}
+
+	dprintf(fd, "global.bootm.verify=%s\n", verify);
+
+	dprintf(fd, "\n");
+
+	if (data->os_address != UIMAGE_SOME_ADDRESS)
+		dprintf(fd, "global.bootm.image.loadaddr=0x%08lx\n", data->os_address);
+	if (data->os_file)
+		dprintf(fd, "global.bootm.image='%s'\n", data->os_file);
+	if (data->oftree_file)
+		dprintf(fd, "global.bootm.oftree='%s'\n", data->oftree_file);
+#ifdef CONFIG_BOOTM_INITRD
+	if (data->initrd_file)
+		dprintf(fd, "global.bootm.initrd='%s'\n", data->initrd_file);
+
+	if (data->initrd_address != UIMAGE_INVALID_ADDRESS)
+		dprintf(fd, "global.bootm.initrd.loadaddr=0x%08lx\n", data->initrd_address);
+#endif
+	if (data->tee_file)
+		dprintf(fd, "global.bootm.tee_file='%s'\n", data->tee_file);
+
+	dprintf(fd, "\n");
+
+	bootargs = linux_bootargs_get();
+	if (bootargs)
+		dprintf(fd, "global linux.bootargs.dyn.concatenated='%s'\n", bootargs);
+
+	if (data->force || data->os_entry != UIMAGE_SOME_ADDRESS) {
+		/* We have no global variables for these two parameters yet, so
+		 * we just print a comment to alert the user to this fact
+		 */
+
+		dprintf(fd, "\n# bootm ");
+		if (data->force)
+				dprintf(fd, "-f ");
+		if (data->os_entry != UIMAGE_SOME_ADDRESS)
+				dprintf(fd, "-e %08lx ", data->os_entry);
+		dputc(fd, '\n');
+	}
+
+	if (file)
+		close(fd);
+
+	return 0;
+}
+
 static int do_bootm(int argc, char *argv[])
 {
 	int opt;
diff --git a/common/bootm.c b/common/bootm.c
index 29ea13e28cb8..3ecd788e7b16 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -825,8 +825,16 @@ int bootm_boot(struct bootm_data *bootm_data)
 	}
 
 	ret = handler->bootm(data);
-	if (data->dryrun)
+	if (data->dryrun) {
 		pr_info("Dryrun. Aborted\n");
+		if (bootm_verbose(data)) {
+			const char *file = "/env/boot/cancelled";
+			if (!bootm_print_cmd(file, bootm_data))
+				pr_info("Boot script written to %s\n", file);
+			else
+				pr_warn("Boot script write to %s failed: %m\n", file);
+		}
+	}
 
 err_out:
 	if (data->os_res)
diff --git a/include/bootm.h b/include/bootm.h
index ee2b574521db..53d2905ca992 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -41,6 +41,15 @@ struct bootm_data {
 
 int bootm_boot(struct bootm_data *data);
 
+#ifdef CONFIG_CMD_BOOTM
+int bootm_print_cmd(const char *file, const struct bootm_data *data);
+#else
+static inline int bootm_print_cmd(const char *file, const struct bootm_data *data)
+{
+	return -ENOSYS;
+}
+#endif
+
 struct image_data {
 	/* simplest case. barebox has already loaded the os here */
 	struct resource *os_res;
-- 
2.39.2

Re: [PATCH RFC] bootm: support printing bootm parameters determined at runtime to file

[Sascha Hauer]

On Wed, Jan 10, 2024 at 03:39:58PM +0100, Ahmad Fatoum wrote:
> The barebox bootm command is often not called directly, but via
> bootloader spec or FIT image boot handlers. For debugging, it can be
> useful to reuse those boot handlers, but replace single artifacts, e.g.
> using the barebox device tree instead of the bootloader-spec provided
> device tree.
> 
> To make this easier, have boot -v -d (verbose + dry run) write a boot
> script that reproduces the cancelled boot to /env/boot/cancelled.

I like the idea very much. I also had this problem more than once.

I think an explicit option rather than "-v -d" would be better. With
this there would be a natural way to document this behaviour in the
boot help output.

/env/boot/cancelled would be saved in the environment with a saveenv
which is likely not desirable. Maybe better put it somewhere in /tmp/?

> +	dprintf(fd, "global.bootm.verify=%s\n", verify);
> +
> +	dprintf(fd, "\n");
> +
> +	if (data->os_address != UIMAGE_SOME_ADDRESS)
> +		dprintf(fd, "global.bootm.image.loadaddr=0x%08lx\n", data->os_address);
> +	if (data->os_file)
> +		dprintf(fd, "global.bootm.image='%s'\n", data->os_file);
> +	if (data->oftree_file)
> +		dprintf(fd, "global.bootm.oftree='%s'\n", data->oftree_file);
> +#ifdef CONFIG_BOOTM_INITRD
> +	if (data->initrd_file)
> +		dprintf(fd, "global.bootm.initrd='%s'\n", data->initrd_file);
> +
> +	if (data->initrd_address != UIMAGE_INVALID_ADDRESS)
> +		dprintf(fd, "global.bootm.initrd.loadaddr=0x%08lx\n", data->initrd_address);
> +#endif
> +	if (data->tee_file)
> +		dprintf(fd, "global.bootm.tee_file='%s'\n", data->tee_file);
> +
> +	dprintf(fd, "\n");
> +
> +	bootargs = linux_bootargs_get();
> +	if (bootargs)
> +		dprintf(fd, "global linux.bootargs.dyn.concatenated='%s'\n", bootargs);

This will contain all of linux.bootargs.* which are then added again
when we actually boot the script, so we end up having many bootargs
twice. This is not very nice, I don't know though how/if we can improve
that.

Sascha

-- 
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 RFC] bootm: support printing bootm parameters determined at runtime to file

[Ahmad Fatoum]

Hello Sascha,

On 10.01.24 16:18, Sascha Hauer wrote:
> On Wed, Jan 10, 2024 at 03:39:58PM +0100, Ahmad Fatoum wrote:
>> The barebox bootm command is often not called directly, but via
>> bootloader spec or FIT image boot handlers. For debugging, it can be
>> useful to reuse those boot handlers, but replace single artifacts, e.g.
>> using the barebox device tree instead of the bootloader-spec provided
>> device tree.
>>
>> To make this easier, have boot -v -d (verbose + dry run) write a boot
>> script that reproduces the cancelled boot to /env/boot/cancelled.
> 
> I like the idea very much. I also had this problem more than once.
> 
> I think an explicit option rather than "-v -d" would be better. With
> this there would be a natural way to document this behaviour in the
> boot help output.
> 
> /env/boot/cancelled would be saved in the environment with a saveenv
> which is likely not desirable. Maybe better put it somewhere in /tmp/?

Or have the new option take an argument, which would be the path to save
the file to?

> 
>> +	dprintf(fd, "global.bootm.verify=%s\n", verify);
>> +
>> +	dprintf(fd, "\n");
>> +
>> +	if (data->os_address != UIMAGE_SOME_ADDRESS)
>> +		dprintf(fd, "global.bootm.image.loadaddr=0x%08lx\n", data->os_address);
>> +	if (data->os_file)
>> +		dprintf(fd, "global.bootm.image='%s'\n", data->os_file);
>> +	if (data->oftree_file)
>> +		dprintf(fd, "global.bootm.oftree='%s'\n", data->oftree_file);
>> +#ifdef CONFIG_BOOTM_INITRD
>> +	if (data->initrd_file)
>> +		dprintf(fd, "global.bootm.initrd='%s'\n", data->initrd_file);
>> +
>> +	if (data->initrd_address != UIMAGE_INVALID_ADDRESS)
>> +		dprintf(fd, "global.bootm.initrd.loadaddr=0x%08lx\n", data->initrd_address);
>> +#endif
>> +	if (data->tee_file)
>> +		dprintf(fd, "global.bootm.tee_file='%s'\n", data->tee_file);
>> +
>> +	dprintf(fd, "\n");
>> +
>> +	bootargs = linux_bootargs_get();
>> +	if (bootargs)
>> +		dprintf(fd, "global linux.bootargs.dyn.concatenated='%s'\n", bootargs);
> 
> This will contain all of linux.bootargs.* which are then added again
> when we actually boot the script, so we end up having many bootargs
> twice. This is not very nice, I don't know though how/if we can improve
> that.

Yes. I could iterate over all linux.bootargs.* instead of adding a new variable.

Cheers,
Ahmad

> 
> Sascha
> 

-- 
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 RFC] bootm: support printing bootm parameters determined at runtime to file

[Sascha Hauer]

On Wed, Jan 10, 2024 at 04:26:11PM +0100, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 10.01.24 16:18, Sascha Hauer wrote:
> > On Wed, Jan 10, 2024 at 03:39:58PM +0100, Ahmad Fatoum wrote:
> >> The barebox bootm command is often not called directly, but via
> >> bootloader spec or FIT image boot handlers. For debugging, it can be
> >> useful to reuse those boot handlers, but replace single artifacts, e.g.
> >> using the barebox device tree instead of the bootloader-spec provided
> >> device tree.
> >>
> >> To make this easier, have boot -v -d (verbose + dry run) write a boot
> >> script that reproduces the cancelled boot to /env/boot/cancelled.
> > 
> > I like the idea very much. I also had this problem more than once.
> > 
> > I think an explicit option rather than "-v -d" would be better. With
> > this there would be a natural way to document this behaviour in the
> > boot help output.
> > 
> > /env/boot/cancelled would be saved in the environment with a saveenv
> > which is likely not desirable. Maybe better put it somewhere in /tmp/?
> 
> Or have the new option take an argument, which would be the path to save
> the file to?

We could make that argument optional, then we could still fall back to a
default when no path is given.

Sascha

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