Hi Zhaofeng,
On Sat, 16 Oct 2021 at 00:16, Zhaofeng Li <he...@zhaofeng.li> wrote:
>
> bootm and zboot accept different arguments:
>
> > bootm [addr [arg ...]]
> > - boot application image stored in memory
> > passing arguments 'arg ...'; when booting a Linux kernel,
> > 'arg' can be the address of an initrd image
>
> > zboot [addr] [size] [initrd addr] [initrd size] [setup] [cmdline]
> > addr - The optional starting address of the bzimage.
> > If not set it defaults to the environment
> > variable "fileaddr".
> > size - The optional size of the bzimage. Defaults to
> > zero.
> > initrd addr - The address of the initrd image to use, if any.
> > initrd size - The size of the initrd image to use, if any.
>
> In the zboot flow, the current code will reuse the bootm args and attempt
> to pass the initrd arg (argv[2]) as the kernel size (should be argv[3]).
> zboot also expects the initrd address and size to be separate arguments.
>
> Let's untangle them and have separate argv/argc locals.
>
> Signed-off-by: Zhaofeng Li <he...@zhaofeng.li>
> Cc: Simon Glass <s...@chromium.org>
> Cc: Bin Meng <bmeng...@gmail.com>
>
> diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c
> index 067c24e5ff..78ebfdcc79 100644
> --- a/cmd/pxe_utils.c
> +++ b/cmd/pxe_utils.c
> @@ -441,11 +441,14 @@ skip_overlay:
> static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label)
> {
> char *bootm_argv[] = { "bootm", NULL, NULL, NULL, NULL };
> + char *zboot_argv[] = { "zboot", NULL, "0", NULL, NULL };
> char initrd_str[28];
> + char initrd_filesize[10];
> char mac_str[29] = "";
> char ip_str[68] = "";
> char *fit_addr = NULL;
> int bootm_argc = 2;
> + int zboot_argc = 3;
> int len = 0;
> ulong kernel_addr;
> void *buf;
> @@ -478,6 +481,11 @@ static int label_boot(struct cmd_tbl *cmdtp, struct
> pxe_label *label)
> strcat(bootm_argv[2], ":");
> strncat(bootm_argv[2], env_get("filesize"), 9);
> bootm_argc = 3;
> +
> + strncpy(initrd_filesize, env_get("filesize"), 9);
> + zboot_argv[3] = env_get("ramdisk_addr_r");
> + zboot_argv[4] = initrd_filesize;
> + zboot_argc = 5;
> }
>
> if (get_relfile_envaddr(cmdtp, label->kernel, "kernel_addr_r") < 0) {
> @@ -529,6 +537,8 @@ static int label_boot(struct cmd_tbl *cmdtp, struct
> pxe_label *label)
> }
>
> bootm_argv[1] = env_get("kernel_addr_r");
> + zboot_argv[1] = env_get("kernel_addr_r");
> +
> /* for FIT, append the configuration identifier */
> if (label->config) {
> int len = strlen(bootm_argv[1]) + strlen(label->config) + 1;
> @@ -665,7 +675,7 @@ static int label_boot(struct cmd_tbl *cmdtp, struct
> pxe_label *label)
> do_bootz(cmdtp, 0, bootm_argc, bootm_argv);
> /* Try booting an x86_64 Linux kernel image */
> else if (IS_ENABLED(CONFIG_CMD_ZBOOT))
> - do_zboot_parent(cmdtp, 0, bootm_argc, bootm_argv, NULL);
> + do_zboot_parent(cmdtp, 0, zboot_argc, zboot_argv, NULL);
>
> unmap_sysmem(buf);
>
> --
> 2.33.0
>
The real fix here will be when we can call the boot/zimage code more
directly, rather than via the CLI.
Would it be worth adding more local variables for the strings being
created? We already have initrd_str and mac_str, for example. We could
add one for kernel_addr and ramdisk_addr:
char *ramdisk_addr
ramdisk_addr = env_get("ramdisk_addr_r")
It means that the bootm_argv[1] and bootm_argv[2] references would be
replaced by local variables.
Then, at the end of the function, create the bootm_argv or zboot array:
bootm_argv[1] = kernel_addr;
bootm_argv[2] = ramdisk_addr
Maybe this is too painful and we should just wait for a change to
directly call the API?
So I'll add my review tag since I think this patch is reasonable.
Reviewed-by: Simon Glass <s...@chromium.org>
Regards,
SImon
