Re: [PATCH v2 1/3] hw/riscv: spike: Allow using binary firmware as bios

[Alistair Francis <alistair23@gmail.com>]

On Tue, Jan 18, 2022 at 9:24 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Currently, we have to use OpenSBI firmware ELF as bios for the spike
> machine because the HTIF console requires ELF for parsing "fromhost"
> and "tohost" symbols.
>
> The latest OpenSBI can now optionally pick-up HTIF register address
> from HTIF DT node so using this feature spike machine can now use
> OpenSBI firmware BIN as bios.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  hw/char/riscv_htif.c         | 33 +++++++++++++++++++----------
>  hw/riscv/spike.c             | 41 ++++++++++++++++++++++--------------
>  include/hw/char/riscv_htif.h |  5 ++++-
>  include/hw/riscv/spike.h     |  1 +
>  4 files changed, 52 insertions(+), 28 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index ddae738d56..729edbf968 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -228,13 +228,25 @@ static const MemoryRegionOps htif_mm_ops = {
>      .write = htif_mm_write,
>  };
>
> +bool htif_uses_elf_symbols(void)
> +{
> +    return (address_symbol_set == 3) ? true : false;
> +}
> +
>  HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr)
> +    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base)
>  {
> -    uint64_t base = MIN(tohost_addr, fromhost_addr);
> -    uint64_t size = MAX(tohost_addr + 8, fromhost_addr + 8) - base;
> -    uint64_t tohost_offset = tohost_addr - base;
> -    uint64_t fromhost_offset = fromhost_addr - base;
> +    uint64_t base, size, tohost_offset, fromhost_offset;
> +
> +    if (!htif_uses_elf_symbols()) {
> +        fromhost_addr = nonelf_base;
> +        tohost_addr = nonelf_base + 8;
> +    }
> +
> +    base = MIN(tohost_addr, fromhost_addr);
> +    size = MAX(tohost_addr + 8, fromhost_addr + 8) - base;
> +    tohost_offset = tohost_addr - base;
> +    fromhost_offset = fromhost_addr - base;
>
>      HTIFState *s = g_malloc0(sizeof(HTIFState));
>      s->address_space = address_space;
> @@ -249,12 +261,11 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
>      qemu_chr_fe_init(&s->chr, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
>          htif_be_change, s, NULL, true);
> -    if (address_symbol_set == 3) {
> -        memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
> -                              TYPE_HTIF_UART, size);
> -        memory_region_add_subregion_overlap(address_space, base,
> -                                            &s->mmio, 1);
> -    }
> +
> +    memory_region_init_io(&s->mmio, NULL, &htif_mm_ops, s,
> +                          TYPE_HTIF_UART, size);
> +    memory_region_add_subregion_overlap(address_space, base,
> +                                        &s->mmio, 1);
>
>      return s;
>  }
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 288d69cd9f..597df4c288 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -42,6 +42,7 @@
>
>  static const MemMapEntry spike_memmap[] = {
>      [SPIKE_MROM] =     {     0x1000,     0xf000 },
> +    [SPIKE_HTIF] =     {  0x1000000,     0x1000 },
>      [SPIKE_CLINT] =    {  0x2000000,    0x10000 },
>      [SPIKE_DRAM] =     { 0x80000000,        0x0 },
>  };
> @@ -75,6 +76,10 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/htif");
>      qemu_fdt_setprop_string(fdt, "/htif", "compatible", "ucb,htif0");
> +    if (!htif_uses_elf_symbols()) {
> +        qemu_fdt_setprop_cells(fdt, "/htif", "reg",
> +            0x0, memmap[SPIKE_HTIF].base, 0x0, memmap[SPIKE_HTIF].size);
> +    }
>
>      qemu_fdt_add_subnode(fdt, "/soc");
>      qemu_fdt_setprop(fdt, "/soc", "ranges", NULL, 0);
> @@ -172,6 +177,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>      if (cmdline) {
>          qemu_fdt_add_subnode(fdt, "/chosen");
>          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> +        qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", "/htif");
>      }
>  }
>
> @@ -241,10 +247,6 @@ static void spike_board_init(MachineState *machine)
>      memory_region_add_subregion(system_memory, memmap[SPIKE_DRAM].base,
>          machine->ram);
>
> -    /* create device tree */
> -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> -               riscv_is_32bit(&s->soc[0]));
> -
>      /* boot rom */
>      memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
>                             memmap[SPIKE_MROM].size, &error_fatal);
> @@ -266,6 +268,7 @@ static void spike_board_init(MachineState *machine)
>                                      htif_symbol_callback);
>      }
>
> +    /* Load kernel */
>      if (machine->kernel_filename) {
>          kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
>                                                           firmware_end_addr);
> @@ -273,17 +276,6 @@ static void spike_board_init(MachineState *machine)
>          kernel_entry = riscv_load_kernel(machine->kernel_filename,
>                                           kernel_start_addr,
>                                           htif_symbol_callback);
> -
> -        if (machine->initrd_filename) {
> -            hwaddr start;
> -            hwaddr end = riscv_load_initrd(machine->initrd_filename,
> -                                           machine->ram_size, kernel_entry,
> -                                           &start);
> -            qemu_fdt_setprop_cell(s->fdt, "/chosen",
> -                                  "linux,initrd-start", start);
> -            qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
> -                                  end);
> -        }
>      } else {
>         /*
>          * If dynamic firmware is used, it doesn't know where is the next mode
> @@ -292,6 +284,22 @@ static void spike_board_init(MachineState *machine)
>          kernel_entry = 0;
>      }
>
> +    /* Create device tree */
> +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> +               riscv_is_32bit(&s->soc[0]));
> +
> +    /* Load initrd */
> +    if (machine->kernel_filename && machine->initrd_filename) {
> +        hwaddr start;
> +        hwaddr end = riscv_load_initrd(machine->initrd_filename,
> +                                       machine->ram_size, kernel_entry,
> +                                       &start);
> +        qemu_fdt_setprop_cell(s->fdt, "/chosen",
> +                              "linux,initrd-start", start);
> +        qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
> +                              end);
> +    }
> +
>      /* Compute the fdt load address in dram */
>      fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
>                                     machine->ram_size, s->fdt);
> @@ -303,7 +311,8 @@ static void spike_board_init(MachineState *machine)
>
>      /* initialize HTIF using symbols found in load_kernel */
>      htif_mm_init(system_memory, mask_rom,
> -                 &s->soc[0].harts[0].env, serial_hd(0));
> +                 &s->soc[0].harts[0].env, serial_hd(0),
> +                 memmap[SPIKE_HTIF].base);
>  }
>
>  static void spike_machine_instance_init(Object *obj)
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index fb9452cf51..f888ac1b30 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -52,8 +52,11 @@ extern const MemoryRegionOps htif_io_ops;
>  void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
>      uint64_t st_size);
>
> +/* Check if HTIF uses ELF symbols */
> +bool htif_uses_elf_symbols(void);
> +
>  /* legacy pre qom */
>  HTIFState *htif_mm_init(MemoryRegion *address_space, MemoryRegion *main_mem,
> -    CPURISCVState *env, Chardev *chr);
> +    CPURISCVState *env, Chardev *chr, uint64_t nonelf_base);
>
>  #endif
> diff --git a/include/hw/riscv/spike.h b/include/hw/riscv/spike.h
> index cdd1a13011..73d69234de 100644
> --- a/include/hw/riscv/spike.h
> +++ b/include/hw/riscv/spike.h
> @@ -43,6 +43,7 @@ struct SpikeState {
>
>  enum {
>      SPIKE_MROM,
> +    SPIKE_HTIF,
>      SPIKE_CLINT,
>      SPIKE_DRAM
>  };
> --
> 2.25.1
>
>