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

[Bin Meng]

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

> +{
> +    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

> +
> +    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

>          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.

>  #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

>      };
>
>      /* 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