Re: [PATCH v5 07/11] hw/riscv: write bootargs 'chosen' FDT after riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The sifive_u, spike and virt machines are writing the 'bootargs' FDT
> node during their respective create_fdt().
>
> Given that bootargs is written only when '-append' is used, and this
> option is only allowed with the '-kernel' option, which in turn is
> already being check before executing riscv_load_kernel(), write
> 'bootargs' in the same code path as riscv_load_kernel().
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/sifive_u.c | 11 +++++------
>  hw/riscv/spike.c    |  9 +++++----
>  hw/riscv/virt.c     | 11 +++++------
>  3 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 37f5087172..3e6df87b5b 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -117,7 +117,6 @@ static void create_fdt(SiFiveUState *s, const MemMapEntry 
> *memmap,
>              error_report("load_device_tree() failed");
>              exit(1);
>          }
> -        goto update_bootargs;
>      } else {
>          fdt = ms->fdt = create_device_tree(&fdt_size);
>          if (!fdt) {
> @@ -510,11 +509,6 @@ static void create_fdt(SiFiveUState *s, const 
> MemMapEntry *memmap,
>      qemu_fdt_setprop_string(fdt, "/aliases", "serial0", nodename);
>
>      g_free(nodename);
> -
> -update_bootargs:
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -    }
>  }
>
>  static void sifive_u_machine_reset(void *opaque, int n, int level)
> @@ -611,6 +605,11 @@ static void sifive_u_machine_init(MachineState *machine)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 5668fe0694..60e2912be5 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -179,10 +179,6 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
> *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/chosen");
>      qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif");
> -
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -    }
>  }
>
>  static bool spike_test_elf_image(char *filename)
> @@ -319,6 +315,11 @@ static void spike_board_init(MachineState *machine)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5967b136b4..6c946b6def 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1012,7 +1012,6 @@ static void create_fdt(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>              error_report("load_device_tree() failed");
>              exit(1);
>          }
> -        goto update_bootargs;
>      } else {
>          mc->fdt = create_device_tree(&s->fdt_size);
>          if (!mc->fdt) {
> @@ -1050,11 +1049,6 @@ static void create_fdt(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      create_fdt_fw_cfg(s, memmap);
>      create_fdt_pmu(s);
>
> -update_bootargs:
> -    if (cmdline && *cmdline) {
> -        qemu_fdt_setprop_string(mc->fdt, "/chosen", "bootargs", cmdline);
> -    }
> -
>      /* Pass seed to RNG */
>      qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>      qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed", rng_seed, 
> sizeof(rng_seed));
> @@ -1294,6 +1288,11 @@ static void virt_machine_done(Notifier *notifier, void 
> *data)
>              riscv_load_initrd(machine->initrd_filename, machine->ram_size,
>                                kernel_entry, machine->fdt);
>          }
> +
> +        if (machine->kernel_cmdline && *machine->kernel_cmdline) {
> +            qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",
> +                                    machine->kernel_cmdline);
> +        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> --
> 2.39.0
>
>