* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.