| Message ID | 20211016061607.215571-1-hello@zhaofeng.li |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | pxe_utils: Fix arguments to x86 zboot | expand |
Hi Zhaofeng, On Sat, 16 Oct 2021 at 00:16, Zhaofeng Li <hello@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 <hello@zhaofeng.li> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@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 <sjg@chromium.org> Regards, SImon
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);