Re: [PATCH v1 07/16] hw/riscv: sifive_u: Remove compile time XLEN checks

[Alistair Francis]

On Mon, Oct 26, 2020 at 1:56 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Oct 23, 2020 at 11:44 PM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/sifive_u.c | 57 ++++++++++++++++++++++++---------------------
> >  1 file changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 978cfedd24..3bc32c9225 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -60,12 +60,6 @@
> >
> >  #include <libfdt.h>
> >
> > -#if defined(TARGET_RISCV32)
> > -# define BIOS_FILENAME "opensbi-riscv32-generic-fw_dynamic.bin"
> > -#else
> > -# define BIOS_FILENAME "opensbi-riscv64-generic-fw_dynamic.bin"
> > -#endif
> > -
> >  static const struct MemmapEntry {
> >      hwaddr base;
> >      hwaddr size;
> > @@ -93,7 +87,7 @@ static const struct MemmapEntry {
> >  #define GEM_REVISION        0x10070109
> >
> >  static void create_fdt(SiFiveUState *s, const struct MemmapEntry *memmap,
> > -    uint64_t mem_size, const char *cmdline)
> > +                       uint64_t mem_size, const char *cmdline, bool 
> > is_32_bit)
> >  {
> >      MachineState *ms = MACHINE(qdev_get_machine());
> >      void *fdt;
> > @@ -167,11 +161,11 @@ static void create_fdt(SiFiveUState *s, const struct 
> > MemmapEntry *memmap,
> >          qemu_fdt_add_subnode(fdt, nodename);
> >          /* cpu 0 is the management hart that does not have mmu */
> >          if (cpu != 0) {
> > -#if defined(TARGET_RISCV32)
> > -            qemu_fdt_setprop_string(fdt, nodename, "mmu-type", 
> > "riscv,sv32");
> > -#else
> > -            qemu_fdt_setprop_string(fdt, nodename, "mmu-type", 
> > "riscv,sv48");
> > -#endif
> > +            if (is_32_bit) {
> > +                qemu_fdt_setprop_string(fdt, nodename, "mmu-type", 
> > "riscv,sv32");
> > +            } else {
> > +                qemu_fdt_setprop_string(fdt, nodename, "mmu-type", 
> > "riscv,sv48");
> > +            }
> >              isa = riscv_isa_string(&s->soc.u_cpus.harts[cpu - 1]);
> >          } else {
> >              isa = riscv_isa_string(&s->soc.e_cpus.harts[0]);
> > @@ -414,7 +408,7 @@ static void sifive_u_machine_init(MachineState *machine)
> >      MemoryRegion *system_memory = get_system_memory();
> >      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >      MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> > -    target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> > +    uint64_t start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
>
> Why is this change necessary? If yes, should we do the same change to virt.c?

Good catch, this isn't required.

I have removed this diff.

Alistair

>
> >      target_ulong firmware_end_addr, kernel_start_addr;
> >      uint32_t start_addr_hi32 = 0x00000000;
> >      int i;
> > @@ -446,7 +440,8 @@ static void sifive_u_machine_init(MachineState *machine)
> >                            qemu_allocate_irq(sifive_u_machine_reset, NULL, 
> > 0));
> >
> >      /* create device tree */
> > -    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> > +    create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> > +               riscv_is_32_bit(machine));
> >
> >      if (s->start_in_flash) {
> >          /*
> > @@ -475,8 +470,15 @@ static void sifive_u_machine_init(MachineState 
> > *machine)
> >          break;
> >      }
> >
> > -    firmware_end_addr = riscv_find_and_load_firmware(machine, 
> > BIOS_FILENAME,
> > -                                                     start_addr, NULL);
> > +    if (riscv_is_32_bit(machine)) {
> > +        firmware_end_addr = riscv_find_and_load_firmware(machine,
> > +                                    
> > "opensbi-riscv32-generic-fw_dynamic.bin",
> > +                                    start_addr, NULL);
> > +    } else {
> > +        firmware_end_addr = riscv_find_and_load_firmware(machine,
> > +                                    
> > "opensbi-riscv64-generic-fw_dynamic.bin",
> > +                                    start_addr, NULL);
> > +    }
> >
> >      if (machine->kernel_filename) {
> >          kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > @@ -506,9 +508,9 @@ static void sifive_u_machine_init(MachineState *machine)
> >      /* Compute the fdt load address in dram */
> >      fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DEV_DRAM].base,
> >                                     machine->ram_size, s->fdt);
> > -    #if defined(TARGET_RISCV64)
> > -    start_addr_hi32 = start_addr >> 32;
> > -    #endif
> > +    if (!riscv_is_32_bit(machine)) {
> > +        start_addr_hi32 = start_addr >> 32;
> > +    }
> >
> >      /* reset vector */
> >      uint32_t reset_vec[11] = {
> > @@ -516,13 +518,8 @@ static void sifive_u_machine_init(MachineState 
> > *machine)
> >          0x00000297,                    /* 1:  auipc  t0, %pcrel_hi(fw_dyn) 
> > */
> >          0x02828613,                    /*     addi   a2, t0, %pcrel_lo(1b) 
> > */
> >          0xf1402573,                    /*     csrr   a0, mhartid  */
> > -#if defined(TARGET_RISCV32)
> > -        0x0202a583,                    /*     lw     a1, 32(t0) */
> > -        0x0182a283,                    /*     lw     t0, 24(t0) */
> > -#elif defined(TARGET_RISCV64)
> > -        0x0202b583,                    /*     ld     a1, 32(t0) */
> > -        0x0182b283,                    /*     ld     t0, 24(t0) */
> > -#endif
> > +        0,
> > +        0,
> >          0x00028067,                    /*     jr     t0 */
> >          start_addr,                    /* start: .dword */
> >          start_addr_hi32,
> > @@ -530,6 +527,14 @@ static void sifive_u_machine_init(MachineState 
> > *machine)
> >          0x00000000,
> >                                         /* fw_dyn: */
> >      };
> > +    if (riscv_is_32_bit(machine)) {
> > +        reset_vec[4] = 0x0202a583;     /*     lw     a1, 32(t0) */
> > +        reset_vec[5] = 0x0182a283;     /*     lw     t0, 24(t0) */
> > +    } else {
> > +        reset_vec[4] = 0x0202b583;     /*     ld     a1, 32(t0) */
> > +        reset_vec[5] = 0x0182b283;     /*     ld     t0, 24(t0) */
> > +    }
> > +
> >
> >      /* copy in the reset vector in little_endian byte order */
> >      for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
>
> Regards,
> Bin