On Tue, Jan 18, 2022 at 3:27 PM Anup Patel <a...@brainfault.org> wrote:
>
> On Tue, Jan 18, 2022 at 10:50 AM Alistair Francis <alistai...@gmail.com>
> wrote:
> >
> > On Sat, Jan 15, 2022 at 2:18 AM Anup Patel <apa...@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.
> >
> > Cool!
> >
> > I think that means we can remove the elfs from QEMU.
>
> Yes, we can definitely remove the ELFs from QEMU.
Ha, I mostly meant can you do that? :P
Alistair
>
> >
> > >
> > > Signed-off-by: Anup Patel <apa...@ventanamicro.com>
> > > ---
> > > 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..b59d321fb7 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 (address_symbol_set != 3) {
> >
> > Why not use htif_uses_elf_symbols() ?
>
> Ahh, yes.
>
> I should have used htif_uses_elf_symbols() here. I will update
> it in the next revision.
>
> Regards,
> Anup
>
> >
> > Alistair
> >
> > > + 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
> > >
> > >
