[PATCH] pxe_utils: Fix arguments to x86 zboot

[Simon Glass]

Hi Zhaofeng,

On Sat, 16 Oct 2021 at 00:16, Zhaofeng Li <hello at 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 at zhaofeng.li>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Bin Meng <bmeng.cn at 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 at chromium.org>

Regards,
SImon