* [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot
@ 2021-10-20 7:18 Zhaofeng Li
2021-10-20 7:18 ` [PATCH v2 1/2] " Zhaofeng Li
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Zhaofeng Li @ 2021-10-20 7:18 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass, Bin Meng, Zhaofeng Li
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(-)
--
2.33.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] pxe_utils: Fix arguments to x86 zboot
2021-10-20 7:18 [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Zhaofeng Li
@ 2021-10-20 7:18 ` Zhaofeng Li
2021-10-24 19:53 ` Simon Glass
2021-10-20 7:18 ` [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation Zhaofeng Li
2021-10-27 3:24 ` [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Bin Meng
2 siblings, 1 reply; 6+ messages in thread
From: Zhaofeng Li @ 2021-10-20 7:18 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass, Bin Meng, Zhaofeng Li
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);
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation
2021-10-20 7:18 [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Zhaofeng Li
2021-10-20 7:18 ` [PATCH v2 1/2] " Zhaofeng Li
@ 2021-10-20 7:18 ` Zhaofeng Li
2021-10-24 19:53 ` Simon Glass
2021-10-27 3:24 ` [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Bin Meng
2 siblings, 1 reply; 6+ messages in thread
From: Zhaofeng Li @ 2021-10-20 7:18 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass, Bin Meng, Zhaofeng Li
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);
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] pxe_utils: Fix arguments to x86 zboot
2021-10-20 7:18 ` [PATCH v2 1/2] " Zhaofeng Li
@ 2021-10-24 19:53 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2021-10-24 19:53 UTC (permalink / raw)
To: Zhaofeng Li; +Cc: U-Boot Mailing List, Bin Meng
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation
2021-10-20 7:18 ` [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation Zhaofeng Li
@ 2021-10-24 19:53 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2021-10-24 19:53 UTC (permalink / raw)
To: Zhaofeng Li; +Cc: U-Boot Mailing List, Bin Meng
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot
2021-10-20 7:18 [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Zhaofeng Li
2021-10-20 7:18 ` [PATCH v2 1/2] " Zhaofeng Li
2021-10-20 7:18 ` [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation Zhaofeng Li
@ 2021-10-27 3:24 ` Bin Meng
2 siblings, 0 replies; 6+ messages in thread
From: Bin Meng @ 2021-10-27 3:24 UTC (permalink / raw)
To: Zhaofeng Li; +Cc: U-Boot Mailing List, Simon Glass
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-27 3:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-20 7:18 [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Zhaofeng Li
2021-10-20 7:18 ` [PATCH v2 1/2] " Zhaofeng Li
2021-10-24 19:53 ` Simon Glass
2021-10-20 7:18 ` [PATCH v2 2/2] pxe_utils: Clean up {bootm,zboot}_argv generation Zhaofeng Li
2021-10-24 19:53 ` Simon Glass
2021-10-27 3:24 ` [PATCH v2 0/2] pxe_utils: Fix arguments to x86 zboot Bin Meng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).