Re: [PATCH v3 2/3] RISC-V: Copy the fdt in dram instead of ROM

[Bin Meng]

On Sat, Jun 27, 2020 at 12:37 PM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Fri, Jun 26, 2020 at 7:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Atish,
> >
> > On Sat, Jun 27, 2020 at 12:58 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 4:50 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Atish,
> > > >
> > > > On Fri, Jun 26, 2020 at 8:33 AM Atish Patra <atish.patra@wdc.com> wrote:
> > > > >
> > > > > Currently, the fdt is copied to the ROM after the reset vector. The 
> > > > > firmware
> > > > > has to copy it to DRAM. Instead of this, directly copy the device 
> > > > > tree to a
> > > > > pre-computed dram address. The device tree load address should be as 
> > > > > far as
> > > > > possible from kernel and initrd images. That's why it is kept at the 
> > > > > end of
> > > > > the DRAM or 4GB whichever is lesser.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > >  hw/riscv/boot.c         | 57 
> > > > > +++++++++++++++++++++++++++++------------
> > > > >  hw/riscv/sifive_u.c     | 32 +++++++++++------------
> > > > >  hw/riscv/spike.c        |  7 ++++-
> > > > >  hw/riscv/virt.c         |  7 ++++-
> > > > >  include/hw/riscv/boot.h |  5 +++-
> > > > >  5 files changed, 71 insertions(+), 37 deletions(-)
> > > > >
> > > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > > > > index 482b78147993..02c4018a8105 100644
> > > > > --- a/hw/riscv/boot.c
> > > > > +++ b/hw/riscv/boot.c
> > > > > @@ -159,44 +159,67 @@ hwaddr riscv_load_initrd(const char *filename, 
> > > > > uint64_t mem_size,
> > > > >      return *start + size;
> > > > >  }
> > > > >
> > > > > +hwaddr riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> > > >
> > > > I think we should use uint32_t for the return value type, since it is
> > > > always below 4GiB
> > > >
> > >
> > > You are correct. I will update it. Thanks.
> > > > > +{
> > > > > +    hwaddr temp, fdt_addr;
> > > > > +    hwaddr dram_end = dram_base + mem_size;
> > > > > +    int fdtsize = fdt_totalsize(fdt);
> > > > > +
> > > > > +    if (fdtsize <= 0) {
> > > > > +        error_report("invalid device-tree");
> > > > > +        exit(1);
> > > > > +    }
> > > > > +
> > > > > +    /*
> > > > > +     * We should put fdt as far as possible to avoid kernel/initrd 
> > > > > overwriting
> > > > > +     * its content. But it should be addressable by 32 bit system as 
> > > > > well.
> > > > > +     * Thus, put it at an aligned address that less than fdt size 
> > > > > from end of
> > > > > +     * dram or 4GB whichever is lesser.
> > > > > +     */
> > > > > +    temp = MIN(dram_end, 4096 * MiB);
> > > > > +    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> > > > > +
> > > > > +    fdt_pack(fdt);
> > > > > +    /* copy in the device tree */
> > > > > +    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > >
> > > > Use fdtsize
> > > >
> > >
> > > Sure.
> > > > > +
> > > > > +    rom_add_blob_fixed_as("fdt", fdt, fdtsize, fdt_addr,
> > > > > +                          &address_space_memory);
> > > > > +
> > > > > +    return fdt_addr;
> > > > > +}
> > > > > +
> > > > >  void riscv_setup_rom_reset_vec(hwaddr start_addr, hwaddr rom_base,
> > > > > -                               hwaddr rom_size, void *fdt)
> > > > > +                               hwaddr rom_size,
> > > > > +                               hwaddr fdt_load_addr, void *fdt)
> > > > >  {
> > > > >      int i;
> > > > >      /* reset vector */
> > > > > -    uint32_t reset_vec[8] = {
> > > > > -        0x00000297,                  /* 1:  auipc  t0, 
> > > > > %pcrel_hi(dtb) */
> > > > > -        0x02028593,                  /*     addi   a1, t0, 
> > > > > %pcrel_lo(1b) */
> > > > > +    uint32_t reset_vec[10] = {
> > > > > +        0x00000297,                  /* 1:  auipc  t0, 
> > > > > %pcrel_hi(fw_dyn) */
> > > >
> > > > fw_dync should be introduced in the next patch, so this line should be
> > > > kept unchanged in this patch
> > > >
> > > As we have fdt_laddr, keeping it unchanged may create confusion with
> > > another dtb label.
> > > I will change the label to "end" in the next version.
> > >
> > > > >          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
> > > > >          0x00028067,                  /*     jr     t0 */
> > > > >          0x00000000,
> > > > >          start_addr,                  /* start: .dword */
> > > > >          0x00000000,
> > > > > -                                     /* dtb: */
> > > > > +        fdt_load_addr,               /* fdt_laddr: .dword */
> > > > > +        0x00000000,
> > > > > +                                     /* fw_dyn: */
> > > > >      };
> > > > >
> > > > >      /* copy in the reset vector in little_endian byte order */
> > > > > -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > > > +    for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> > > > >          reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > > >      }
> > > > >      rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > > > >                            rom_base, &address_space_memory);
> > > > >
> > > > > -    /* copy in the device tree */
> > > > > -    if (fdt_pack(fdt) || fdt_totalsize(fdt) >
> > > > > -        rom_size - sizeof(reset_vec)) {
> > > > > -        error_report("not enough space to store device-tree");
> > > > > -        exit(1);
> > > > > -    }
> > > > > -    qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > > > -    rom_add_blob_fixed_as("mrom.fdt", fdt, fdt_totalsize(fdt),
> > > > > -                           rom_base + sizeof(reset_vec),
> > > > > -                           &address_space_memory);
> > > > > -
> > > > >      return;
> > > > >  }
> > > > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > > > > index 395b21703ab4..7d39a4e4ec6d 100644
> > > > > --- a/hw/riscv/sifive_u.c
> > > > > +++ b/hw/riscv/sifive_u.c
> > > > > @@ -379,6 +379,7 @@ static void sifive_u_machine_init(MachineState 
> > > > > *machine)
> > > > >      MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> > > > >      target_ulong start_addr = memmap[SIFIVE_U_DRAM].base;
> > > > >      int i;
> > > > > +    hwaddr fdt_load_addr;
> > > > >
> > > > >      /* Initialize SoC */
> > > > >      object_initialize_child(OBJECT(machine), "soc", &s->soc, 
> > > > > TYPE_RISCV_U_SOC);
> > > > > @@ -450,40 +451,37 @@ static void sifive_u_machine_init(MachineState 
> > > > > *machine)
> > > > >          }
> > > > >      }
> > > > >
> > > > > +    /* Compute the fdt load address in dram */
> > > > > +    fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DRAM].base,
> > > > > +                                   machine->ram_size, s->fdt);
> > > > > +
> > > > >      /* reset vector */
> > > > > -    uint32_t reset_vec[8] = {
> > > > > +    uint32_t reset_vec[11] = {
> > > > >          s->msel,                       /* MSEL pin state */
> > > > > -        0x00000297,                    /* 1:  auipc  t0, 
> > > > > %pcrel_hi(dtb) */
> > > > > -        0x01c28593,                    /*     addi   a1, t0, 
> > > > > %pcrel_lo(1b) */
> > > > > +        0x00000297,                    /* 1:  auipc  t0, 
> > > > > %pcrel_hi(fw_dyn) */
> > > > >          0xf1402573,                    /*     csrr   a0, mhartid  */
> > > > >  #if defined(TARGET_RISCV32)
> > > > > +        0x0202a583,                    /*     lw     a1, 32(t0) */
> > > > >          0x0182a283,                    /*     lw     t0, 24(t0) */
> > > > >  #elif defined(TARGET_RISCV64)
> > > > > -        0x0182e283,                    /*     lwu    t0, 24(t0) */
> > > > > +        0x0202b583,                    /*     ld     a1, 32(t0) */
> > > > > +        0x0182b283,                    /*     ld     t0, 24(t0) */
> > > >
> > > > This change (lwu => ld) is unnecessary.
> > > >
> > > start_addr is a dword. Currently, the start address is within 32 bits.
> > > But it can be changed to more than 32 bits. No ?
> >
> > For RV32, only 32-bit can be used here. For Rv64, I am not sure
> > whether it is a big value to support jumping directly to 4GiB address
> > above in QEMU. For FU540 SoC, all possible boot adddresses are below
> > 4GiB so I think there is no need to support that in QEMU.
> >
>
> That's true only for hifive unleashed. But that may change for future
> generations of sifive socs.
> But I was trying to say that it is confusing to use a lwu for a dword
> (as per the comment section).
> If you prefer to use lwu only, we should cast it to a 32 bit value,
> update the comment and use lwu.
>
> I prefer ld because it is not ambiguous and future proof as well.

If we use ld, then please fix the 0x00000000 entry, so that it becomes
the high 32-bit of start_addr.

>
> > >
> > > > >  #endif
> > > > >          0x00028067,                    /*     jr     t0 */
> > > > >          0x00000000,
> > > > >          start_addr,                    /* start: .dword */
> > > > > -                                       /* dtb: */
> > > > > +        0x00000000,
> > > >
> > > > unnecessary change. Above lwu can be kept unchanged
> > > >
> > > > > +        fdt_load_addr,                 /* fdt_laddr: .dword */
> > > > > +        0x00000000,
> > > > > +                                       /* fw_dyn: */
> > > >
> > > > should be in next patch
> > > >
> > > Will do.
> > >
> > > > >      };
> > > > >
> > > > >      /* copy in the reset vector in little_endian byte order */
> > > > > -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > > > > +    for (i = 0; i < ARRAY_SIZE(reset_vec); i++) {
> > > > >          reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > > > >      }
> > > > >      rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > > > >                            memmap[SIFIVE_U_MROM].base, 
> > > > > &address_space_memory);
> > > > > -
> > > > > -    /* copy in the device tree */
> > > > > -    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > > > > -            memmap[SIFIVE_U_MROM].size - sizeof(reset_vec)) {
> > > > > -        error_report("not enough space to store device-tree");
> > > > > -        exit(1);
> > > > > -    }
> > > > > -    qemu_fdt_dumpdtb(s->fdt, fdt_totalsize(s->fdt));
> > > > > -    rom_add_blob_fixed_as("mrom.fdt", s->fdt, fdt_totalsize(s->fdt),
> > > > > -                          memmap[SIFIVE_U_MROM].base + 
> > > > > sizeof(reset_vec),
> > > > > -                          &address_space_memory);
> > > > >  }
> > > > >
> > > > >  static bool sifive_u_machine_get_start_in_flash(Object *obj, Error 
> > > > > **errp)
> > > > > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > > > > index c696077cbc16..69f050c07e5a 100644
> > > > > --- a/hw/riscv/spike.c
> > > > > +++ b/hw/riscv/spike.c
> > > > > @@ -163,6 +163,7 @@ static void spike_board_init(MachineState 
> > > > > *machine)
> > > > >      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > > > >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > > > >      unsigned int smp_cpus = machine->smp.cpus;
> > > > > +    hwaddr fdt_load_addr;
> > > > >
> > > > >      /* Initialize SOC */
> > > > >      object_initialize_child(OBJECT(machine), "soc", &s->soc,
> > > > > @@ -208,9 +209,13 @@ static void spike_board_init(MachineState 
> > > > > *machine)
> > > > >          }
> > > > >      }
> > > > >
> > > > > +    /* Compute the fdt load address in dram */
> > > > > +    fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
> > > > > +                                   machine->ram_size, s->fdt);
> > > > >      /* load the reset vector */
> > > > >      riscv_setup_rom_reset_vec(memmap[SPIKE_DRAM].base, 
> > > > > memmap[SPIKE_MROM].base,
> > > > > -                              memmap[SPIKE_MROM].size, s->fdt);
> > > > > +                              memmap[SPIKE_MROM].size,
> > > > > +                              fdt_load_addr, s->fdt);
> > > > >
> > > > >      /* initialize HTIF using symbols found in load_kernel */
> > > > >      htif_mm_init(system_memory, mask_rom, &s->soc.harts[0].env, 
> > > > > serial_hd(0));
> > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > > > index 8ec77e43de26..639e284fc2e3 100644
> > > > > --- a/hw/riscv/virt.c
> > > > > +++ b/hw/riscv/virt.c
> > > > > @@ -478,6 +478,7 @@ static void virt_machine_init(MachineState 
> > > > > *machine)
> > > > >      char *plic_hart_config;
> > > > >      size_t plic_hart_config_len;
> > > > >      target_ulong start_addr = memmap[VIRT_DRAM].base;
> > > > > +    hwaddr fdt_load_addr;
> > > > >      int i;
> > > > >      unsigned int smp_cpus = machine->smp.cpus;
> > > > >
> > > > > @@ -532,9 +533,13 @@ static void virt_machine_init(MachineState 
> > > > > *machine)
> > > > >          start_addr = virt_memmap[VIRT_FLASH].base;
> > > > >      }
> > > > >
> > > > > +    /* Compute the fdt load address in dram */
> > > > > +    fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
> > > > > +                                   machine->ram_size, s->fdt);
> > > > >      /* load the reset vector */
> > > > >      riscv_setup_rom_reset_vec(start_addr, 
> > > > > virt_memmap[VIRT_MROM].base,
> > > > > -                              virt_memmap[VIRT_MROM].size, s->fdt);
> > > > > +                              virt_memmap[VIRT_MROM].size,
> > > > > +                              fdt_load_addr, s->fdt);
> > > > >
> > > > >      /* create PLIC hart topology configuration string */
> > > > >      plic_hart_config_len = (strlen(VIRT_PLIC_HART_CONFIG) + 1) * 
> > > > > smp_cpus;
> > > > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > > > > index 3e9759c89aa2..f64fcadd2390 100644
> > > > > --- a/include/hw/riscv/boot.h
> > > > > +++ b/include/hw/riscv/boot.h
> > > > > @@ -35,7 +35,10 @@ target_ulong riscv_load_kernel(const char 
> > > > > *kernel_filename,
> > > > >                                 symbol_fn_t sym_cb);
> > > > >  hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> > > > >                           uint64_t kernel_entry, hwaddr *start);
> > > > > +hwaddr riscv_load_fdt(hwaddr dram_start, uint64_t dram_size,
> > > > > +                                void *fdt);
> > > >
> > > > nits: not indented to (
> > > >
> > > > >  void riscv_setup_rom_reset_vec(hwaddr saddr, hwaddr rom_base,
> > > > > -                               hwaddr rom_size, void *fdt);
> > > > > +                               hwaddr rom_size,
> > > > > +                               hwaddr fdt_load_addr, void *fdt);
> > > > >
> > > > >  #endif /* RISCV_BOOT_H */
> > > >

Regards,
Bin