[PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot

Hi Simon,
Thanks for your review! I have added a second patch to perform the cleanup that you mentioned in the review, so the actual "fix" patch stays minimal and easy to review.
I agree that calling the bootm and zboot code directly is the real solution to go. The current method is inherently error-prone, and I wonder how many cases of "kinda works but not really" [1] like this are there in U-Boot.
Thanks, Zhaofeng Li
[1] Without the patch, the kernel would boot with the U-Boot log showing initrd being loaded. However, the kernel wouldn't actually get the initrd.
---
This patch series fixes the issue that incorrect arguments are passed to x86 zboot in pxe_utils (pxe/extlinux-like config). See the commit message of the first patch for details.
Changes since v1: - Added patch to clean up argv generation
Zhaofeng Li (2): pxe_utils: Fix arguments to x86 zboot pxe_utils: Clean up {bootm,zboot}_argv generation
cmd/pxe_utils.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)

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 --- cmd/pxe_utils.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
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);

On Wed, 20 Oct 2021 at 01:18, 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
cmd/pxe_utils.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org

Signed-off-by: Zhaofeng Li hello@zhaofeng.li Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com --- cmd/pxe_utils.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/cmd/pxe_utils.c b/cmd/pxe_utils.c index 78ebfdcc79..b79fcb6418 100644 --- a/cmd/pxe_utils.c +++ b/cmd/pxe_utils.c @@ -442,15 +442,17 @@ 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 *kernel_addr = NULL; + char *initrd_addr_str = NULL; char initrd_filesize[10]; + char initrd_str[28]; 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; + ulong kernel_addr_r; void *buf;
label_print(label); @@ -476,16 +478,12 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) return 1; }
- bootm_argv[2] = initrd_str; - strncpy(bootm_argv[2], env_get("ramdisk_addr_r"), 18); - strcat(bootm_argv[2], ":"); - strncat(bootm_argv[2], env_get("filesize"), 9); - bootm_argc = 3; - + initrd_addr_str = env_get("ramdisk_addr_r"); strncpy(initrd_filesize, env_get("filesize"), 9); - zboot_argv[3] = env_get("ramdisk_addr_r"); - zboot_argv[4] = initrd_filesize; - zboot_argc = 5; + + strncpy(initrd_str, initrd_addr_str, 18); + strcat(initrd_str, ":"); + strncat(initrd_str, initrd_filesize, 9); }
if (get_relfile_envaddr(cmdtp, label->kernel, "kernel_addr_r") < 0) { @@ -536,20 +534,19 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) printf("append: %s\n", finalbootargs); }
- bootm_argv[1] = env_get("kernel_addr_r"); - zboot_argv[1] = env_get("kernel_addr_r"); + kernel_addr = env_get("kernel_addr_r");
/* for FIT, append the configuration identifier */ if (label->config) { - int len = strlen(bootm_argv[1]) + strlen(label->config) + 1; + int len = strlen(kernel_addr) + strlen(label->config) + 1;
fit_addr = malloc(len); if (!fit_addr) { printf("malloc fail (FIT address)\n"); return 1; } - snprintf(fit_addr, len, "%s%s", bootm_argv[1], label->config); - bootm_argv[1] = fit_addr; + snprintf(fit_addr, len, "%s%s", kernel_addr, label->config); + kernel_addr = fit_addr; }
/* @@ -653,6 +650,18 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) } }
+ bootm_argv[1] = kernel_addr; + zboot_argv[1] = kernel_addr; + + if (initrd_addr_str) { + bootm_argv[2] = initrd_str; + bootm_argc = 3; + + zboot_argv[3] = initrd_addr_str; + zboot_argv[4] = initrd_filesize; + zboot_argc = 5; + } + if (!bootm_argv[3]) bootm_argv[3] = env_get("fdt_addr");
@@ -662,8 +671,8 @@ static int label_boot(struct cmd_tbl *cmdtp, struct pxe_label *label) bootm_argc = 4; }
- kernel_addr = genimg_get_kernel_addr(bootm_argv[1]); - buf = map_sysmem(kernel_addr, 0); + kernel_addr_r = genimg_get_kernel_addr(kernel_addr); + buf = map_sysmem(kernel_addr_r, 0); /* Try bootm for legacy and FIT format image */ if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID) do_bootm(cmdtp, 0, bootm_argc, bootm_argv);

On Wed, 20 Oct 2021 at 01:18, Zhaofeng Li hello@zhaofeng.li wrote:
Signed-off-by: Zhaofeng Li hello@zhaofeng.li Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com
cmd/pxe_utils.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please add a commit message.

On Wed, Oct 20, 2021 at 3:18 PM Zhaofeng Li hello@zhaofeng.li wrote:
Hi Simon,
Thanks for your review! I have added a second patch to perform the cleanup that you mentioned in the review, so the actual "fix" patch stays minimal and easy to review.
I agree that calling the bootm and zboot code directly is the real solution to go. The current method is inherently error-prone, and I wonder how many cases of "kinda works but not really" [1] like this are there in U-Boot.
Thanks, Zhaofeng Li
[1] Without the patch, the kernel would boot with the U-Boot log showing initrd being loaded. However, the kernel wouldn't actually get the initrd.
This patch series fixes the issue that incorrect arguments are passed to x86 zboot in pxe_utils (pxe/extlinux-like config). See the commit message of the first patch for details.
Changes since v1:
- Added patch to clean up argv generation
Zhaofeng Li (2): pxe_utils: Fix arguments to x86 zboot pxe_utils: Clean up {bootm,zboot}_argv generation
cmd/pxe_utils.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
series applied to u-boot-x86, thanks!
participants (3)
-
Bin Meng
-
Simon Glass
-
Zhaofeng Li