[PATCH 1/3] riscv: Unify Qemu's reset vector code path

Atish Patra posted 3 patches 5 years, 7 months ago
Maintainers: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>
There is a newer version of this series
[PATCH 1/3] riscv: Unify Qemu's reset vector code path
Posted by Atish Patra 5 years, 7 months ago
Currently, all riscv machines have identical reset vector code
implementations with memory addresses being different for all machines.
They can be easily combined into a single function in common code.

Move it to common function and let all the machines use the common function.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 hw/riscv/boot.c         | 46 +++++++++++++++++++++++++++++++++++++++++
 hw/riscv/sifive_u.c     | 38 +++-------------------------------
 hw/riscv/spike.c        | 38 +++-------------------------------
 hw/riscv/virt.c         | 37 +++------------------------------
 include/hw/riscv/boot.h |  2 ++
 5 files changed, 57 insertions(+), 104 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index adb421b91b68..8ed96da600c9 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -22,12 +22,16 @@
 #include "qemu/units.h"
 #include "qemu/error-report.h"
 #include "exec/cpu-defs.h"
+#include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "hw/riscv/boot.h"
 #include "elf.h"
+#include "sysemu/device_tree.h"
 #include "sysemu/qtest.h"
 
+#include <libfdt.h>
+
 #if defined(TARGET_RISCV32)
 # define KERNEL_BOOT_ADDRESS 0x80400000
 #else
@@ -155,3 +159,45 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
 
     return *start + size;
 }
+
+void riscv_setup_rom_reset_vec(hwaddr start_addr, hwaddr rom_base,
+                               hwaddr rom_size, 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) */
+        0xf1402573,                  /*     csrr   a0, mhartid  */
+#if defined(TARGET_RISCV32)
+        0x0182a283,                  /*     lw     t0, 24(t0) */
+#elif defined(TARGET_RISCV64)
+        0x0182b283,                  /*     ld     t0, 24(t0) */
+#endif
+        0x00028067,                  /*     jr     t0 */
+        0x00000000,
+        start_addr,                  /* start: .dword */
+        0x00000000,
+                                     /* dtb: */
+    };
+
+    /* copy in the reset vector in little_endian byte order */
+    for (i = 0; i < sizeof(reset_vec) >> 2; 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 f9fef2be9170..c2712570e0d9 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -325,7 +325,6 @@ static void sifive_u_machine_init(MachineState *machine)
     MemoryRegion *main_mem = g_new(MemoryRegion, 1);
     MemoryRegion *flash0 = g_new(MemoryRegion, 1);
     target_ulong start_addr = memmap[SIFIVE_U_DRAM].base;
-    int i;
 
     /* Initialize SoC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc,
@@ -374,40 +373,9 @@ static void sifive_u_machine_init(MachineState *machine)
         start_addr = memmap[SIFIVE_U_FLASH0].base;
     }
 
-    /* reset vector */
-    uint32_t reset_vec[8] = {
-        0x00000297,                    /* 1:  auipc  t0, %pcrel_hi(dtb) */
-        0x02028593,                    /*     addi   a1, t0, %pcrel_lo(1b) */
-        0xf1402573,                    /*     csrr   a0, mhartid  */
-#if defined(TARGET_RISCV32)
-        0x0182a283,                    /*     lw     t0, 24(t0) */
-#elif defined(TARGET_RISCV64)
-        0x0182b283,                    /*     ld     t0, 24(t0) */
-#endif
-        0x00028067,                    /*     jr     t0 */
-        0x00000000,
-        start_addr,                    /* start: .dword */
-        0x00000000,
-                                       /* dtb: */
-    };
-
-    /* copy in the reset vector in little_endian byte order */
-    for (i = 0; i < sizeof(reset_vec) >> 2; 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);
+    /* load the reset vector */
+    riscv_setup_rom_reset_vec(start_addr, memmap[SIFIVE_U_MROM].base,
+                              memmap[SIFIVE_U_MROM].size, s->fdt);
 }
 
 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 7bbbdb50363d..238eae48716a 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -165,7 +165,6 @@ static void spike_board_init(MachineState *machine)
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *main_mem = g_new(MemoryRegion, 1);
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
-    int i;
     unsigned int smp_cpus = machine->smp.cpus;
 
     /* Initialize SOC */
@@ -213,40 +212,9 @@ static void spike_board_init(MachineState *machine)
         }
     }
 
-    /* reset vector */
-    uint32_t reset_vec[8] = {
-        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
-        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
-        0xf1402573,                  /*     csrr   a0, mhartid  */
-#if defined(TARGET_RISCV32)
-        0x0182a283,                  /*     lw     t0, 24(t0) */
-#elif defined(TARGET_RISCV64)
-        0x0182b283,                  /*     ld     t0, 24(t0) */
-#endif
-        0x00028067,                  /*     jr     t0 */
-        0x00000000,
-        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
-        0x00000000,
-                                     /* dtb: */
-    };
-
-    /* copy in the reset vector in little_endian byte order */
-    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
-        reset_vec[i] = cpu_to_le32(reset_vec[i]);
-    }
-    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
-                          memmap[SPIKE_MROM].base, &address_space_memory);
-
-    /* copy in the device tree */
-    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
-            memmap[SPIKE_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[SPIKE_MROM].base + sizeof(reset_vec),
-                          &address_space_memory);
+    /* load the reset vector */
+    riscv_setup_rom_reset_vec(memmap[SPIKE_DRAM].base, memmap[SPIKE_MROM].base,
+                              memmap[SPIKE_MROM].size, 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 4e4c494a7050..a8e2d58cc067 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -536,40 +536,9 @@ static void virt_machine_init(MachineState *machine)
         start_addr = virt_memmap[VIRT_FLASH].base;
     }
 
-    /* reset vector */
-    uint32_t reset_vec[8] = {
-        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
-        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
-        0xf1402573,                  /*     csrr   a0, mhartid  */
-#if defined(TARGET_RISCV32)
-        0x0182a283,                  /*     lw     t0, 24(t0) */
-#elif defined(TARGET_RISCV64)
-        0x0182b283,                  /*     ld     t0, 24(t0) */
-#endif
-        0x00028067,                  /*     jr     t0 */
-        0x00000000,
-        start_addr,                  /* start: .dword */
-        0x00000000,
-                                     /* dtb: */
-    };
-
-    /* copy in the reset vector in little_endian byte order */
-    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
-        reset_vec[i] = cpu_to_le32(reset_vec[i]);
-    }
-    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
-                          memmap[VIRT_MROM].base, &address_space_memory);
-
-    /* copy in the device tree */
-    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
-            memmap[VIRT_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[VIRT_MROM].base + sizeof(reset_vec),
-                          &address_space_memory);
+    /* load the reset vector */
+    riscv_setup_rom_reset_vec(start_addr, virt_memmap[VIRT_MROM].base,
+                              virt_memmap[VIRT_MROM].size, 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 9daa98da08d7..3e9759c89aa2 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -35,5 +35,7 @@ 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);
+void riscv_setup_rom_reset_vec(hwaddr saddr, hwaddr rom_base,
+                               hwaddr rom_size, void *fdt);
 
 #endif /* RISCV_BOOT_H */
-- 
2.26.2


Re: [PATCH 1/3] riscv: Unify Qemu's reset vector code path
Posted by Bin Meng 5 years, 7 months ago
On Wed, Jun 17, 2020 at 3:30 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, all riscv machines have identical reset vector code
> implementations with memory addresses being different for all machines.
> They can be easily combined into a single function in common code.
>
> Move it to common function and let all the machines use the common function.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  hw/riscv/boot.c         | 46 +++++++++++++++++++++++++++++++++++++++++
>  hw/riscv/sifive_u.c     | 38 +++-------------------------------

sifive_u's reset vector has to be different to emulate the real
hardware MSEL pin state.
Please rebase this on top of the following series:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=183567

>  hw/riscv/spike.c        | 38 +++-------------------------------
>  hw/riscv/virt.c         | 37 +++------------------------------
>  include/hw/riscv/boot.h |  2 ++
>  5 files changed, 57 insertions(+), 104 deletions(-)
>

Regards,
Bin

Re: [PATCH 1/3] riscv: Unify Qemu's reset vector code path
Posted by Atish Patra 5 years, 7 months ago
On Thu, Jun 18, 2020 at 1:03 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Jun 17, 2020 at 3:30 AM Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Currently, all riscv machines have identical reset vector code
> > implementations with memory addresses being different for all machines.
> > They can be easily combined into a single function in common code.
> >
> > Move it to common function and let all the machines use the common function.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  hw/riscv/boot.c         | 46 +++++++++++++++++++++++++++++++++++++++++
> >  hw/riscv/sifive_u.c     | 38 +++-------------------------------
>
> sifive_u's reset vector has to be different to emulate the real
> hardware MSEL pin state.
> Please rebase this on top of the following series:
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=183567
>
Sure. I will rebase. I think sifive_u may be used in future to emulate
other sifive boards in future.
This may require additional data in rom. That's why, it's better to
keep the reset vector code in
sifive_u and just unify spike & virt.

> >  hw/riscv/spike.c        | 38 +++-------------------------------
> >  hw/riscv/virt.c         | 37 +++------------------------------
> >  include/hw/riscv/boot.h |  2 ++
> >  5 files changed, 57 insertions(+), 104 deletions(-)
> >
>
> Regards,
> Bin
>


-- 
Regards,
Atish

Re: [PATCH 1/3] riscv: Unify Qemu's reset vector code path
Posted by Alexander Richardson 5 years, 7 months ago
On Tue, 16 Jun 2020 at 20:30, Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, all riscv machines have identical reset vector code
> implementations with memory addresses being different for all machines.
> They can be easily combined into a single function in common code.
>
> Move it to common function and let all the machines use the common function.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  hw/riscv/boot.c         | 46 +++++++++++++++++++++++++++++++++++++++++
>  hw/riscv/sifive_u.c     | 38 +++-------------------------------
>  hw/riscv/spike.c        | 38 +++-------------------------------
>  hw/riscv/virt.c         | 37 +++------------------------------
>  include/hw/riscv/boot.h |  2 ++
>  5 files changed, 57 insertions(+), 104 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index adb421b91b68..8ed96da600c9 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -22,12 +22,16 @@
>  #include "qemu/units.h"
>  #include "qemu/error-report.h"
>  #include "exec/cpu-defs.h"
> +#include "exec/address-spaces.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
>  #include "hw/riscv/boot.h"
>  #include "elf.h"
> +#include "sysemu/device_tree.h"
>  #include "sysemu/qtest.h"
>
> +#include <libfdt.h>
> +
>  #if defined(TARGET_RISCV32)
>  # define KERNEL_BOOT_ADDRESS 0x80400000
>  #else
> @@ -155,3 +159,45 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
>
>      return *start + size;
>  }
> +
> +void riscv_setup_rom_reset_vec(hwaddr start_addr, hwaddr rom_base,
> +                               hwaddr rom_size, 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) */
> +        0xf1402573,                  /*     csrr   a0, mhartid  */
> +#if defined(TARGET_RISCV32)
> +        0x0182a283,                  /*     lw     t0, 24(t0) */
> +#elif defined(TARGET_RISCV64)
> +        0x0182b283,                  /*     ld     t0, 24(t0) */
> +#endif
> +        0x00028067,                  /*     jr     t0 */
> +        0x00000000,
> +        start_addr,                  /* start: .dword */
> +        0x00000000,
> +                                     /* dtb: */
> +    };
> +
> +    /* copy in the reset vector in little_endian byte order */
> +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> +    }
Maybe use ARRAY_SIZE(reset_vec) instead of sizeof(reset_vec) >> 2 ?

> +    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 f9fef2be9170..c2712570e0d9 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -325,7 +325,6 @@ static void sifive_u_machine_init(MachineState *machine)
>      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>      MemoryRegion *flash0 = g_new(MemoryRegion, 1);
>      target_ulong start_addr = memmap[SIFIVE_U_DRAM].base;
> -    int i;
>
>      /* Initialize SoC */
>      object_initialize_child(OBJECT(machine), "soc", &s->soc,
> @@ -374,40 +373,9 @@ static void sifive_u_machine_init(MachineState *machine)
>          start_addr = memmap[SIFIVE_U_FLASH0].base;
>      }
>
> -    /* reset vector */
> -    uint32_t reset_vec[8] = {
> -        0x00000297,                    /* 1:  auipc  t0, %pcrel_hi(dtb) */
> -        0x02028593,                    /*     addi   a1, t0, %pcrel_lo(1b) */
> -        0xf1402573,                    /*     csrr   a0, mhartid  */
> -#if defined(TARGET_RISCV32)
> -        0x0182a283,                    /*     lw     t0, 24(t0) */
> -#elif defined(TARGET_RISCV64)
> -        0x0182b283,                    /*     ld     t0, 24(t0) */
> -#endif
> -        0x00028067,                    /*     jr     t0 */
> -        0x00000000,
> -        start_addr,                    /* start: .dword */
> -        0x00000000,
> -                                       /* dtb: */
> -    };
> -
> -    /* copy in the reset vector in little_endian byte order */
> -    for (i = 0; i < sizeof(reset_vec) >> 2; 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);
> +    /* load the reset vector */
> +    riscv_setup_rom_reset_vec(start_addr, memmap[SIFIVE_U_MROM].base,
> +                              memmap[SIFIVE_U_MROM].size, s->fdt);
>  }
>
>  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 7bbbdb50363d..238eae48716a 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -165,7 +165,6 @@ static void spike_board_init(MachineState *machine)
>      MemoryRegion *system_memory = get_system_memory();
>      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> -    int i;
>      unsigned int smp_cpus = machine->smp.cpus;
>
>      /* Initialize SOC */
> @@ -213,40 +212,9 @@ static void spike_board_init(MachineState *machine)
>          }
>      }
>
> -    /* reset vector */
> -    uint32_t reset_vec[8] = {
> -        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> -        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> -        0xf1402573,                  /*     csrr   a0, mhartid  */
> -#if defined(TARGET_RISCV32)
> -        0x0182a283,                  /*     lw     t0, 24(t0) */
> -#elif defined(TARGET_RISCV64)
> -        0x0182b283,                  /*     ld     t0, 24(t0) */
> -#endif
> -        0x00028067,                  /*     jr     t0 */
> -        0x00000000,
> -        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> -        0x00000000,
> -                                     /* dtb: */
> -    };
> -
> -    /* copy in the reset vector in little_endian byte order */
> -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> -        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> -    }
> -    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> -                          memmap[SPIKE_MROM].base, &address_space_memory);
> -
> -    /* copy in the device tree */
> -    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> -            memmap[SPIKE_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[SPIKE_MROM].base + sizeof(reset_vec),
> -                          &address_space_memory);
> +    /* load the reset vector */
> +    riscv_setup_rom_reset_vec(memmap[SPIKE_DRAM].base, memmap[SPIKE_MROM].base,
> +                              memmap[SPIKE_MROM].size, 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 4e4c494a7050..a8e2d58cc067 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -536,40 +536,9 @@ static void virt_machine_init(MachineState *machine)
>          start_addr = virt_memmap[VIRT_FLASH].base;
>      }
>
> -    /* reset vector */
> -    uint32_t reset_vec[8] = {
> -        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> -        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> -        0xf1402573,                  /*     csrr   a0, mhartid  */
> -#if defined(TARGET_RISCV32)
> -        0x0182a283,                  /*     lw     t0, 24(t0) */
> -#elif defined(TARGET_RISCV64)
> -        0x0182b283,                  /*     ld     t0, 24(t0) */
> -#endif
> -        0x00028067,                  /*     jr     t0 */
> -        0x00000000,
> -        start_addr,                  /* start: .dword */
> -        0x00000000,
> -                                     /* dtb: */
> -    };
> -
> -    /* copy in the reset vector in little_endian byte order */
> -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> -        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> -    }
> -    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> -                          memmap[VIRT_MROM].base, &address_space_memory);
> -
> -    /* copy in the device tree */
> -    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> -            memmap[VIRT_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[VIRT_MROM].base + sizeof(reset_vec),
> -                          &address_space_memory);
> +    /* load the reset vector */
> +    riscv_setup_rom_reset_vec(start_addr, virt_memmap[VIRT_MROM].base,
> +                              virt_memmap[VIRT_MROM].size, 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 9daa98da08d7..3e9759c89aa2 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -35,5 +35,7 @@ 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);
> +void riscv_setup_rom_reset_vec(hwaddr saddr, hwaddr rom_base,
> +                               hwaddr rom_size, void *fdt);
>
>  #endif /* RISCV_BOOT_H */
> --
> 2.26.2
>
>

Re: [PATCH 1/3] riscv: Unify Qemu's reset vector code path
Posted by Atish Patra 5 years, 7 months ago
On Fri, Jun 19, 2020 at 10:11 AM Alexander Richardson
<Alexander.Richardson@cl.cam.ac.uk> wrote:
>
> On Tue, 16 Jun 2020 at 20:30, Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Currently, all riscv machines have identical reset vector code
> > implementations with memory addresses being different for all machines.
> > They can be easily combined into a single function in common code.
> >
> > Move it to common function and let all the machines use the common function.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  hw/riscv/boot.c         | 46 +++++++++++++++++++++++++++++++++++++++++
> >  hw/riscv/sifive_u.c     | 38 +++-------------------------------
> >  hw/riscv/spike.c        | 38 +++-------------------------------
> >  hw/riscv/virt.c         | 37 +++------------------------------
> >  include/hw/riscv/boot.h |  2 ++
> >  5 files changed, 57 insertions(+), 104 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index adb421b91b68..8ed96da600c9 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -22,12 +22,16 @@
> >  #include "qemu/units.h"
> >  #include "qemu/error-report.h"
> >  #include "exec/cpu-defs.h"
> > +#include "exec/address-spaces.h"
> >  #include "hw/boards.h"
> >  #include "hw/loader.h"
> >  #include "hw/riscv/boot.h"
> >  #include "elf.h"
> > +#include "sysemu/device_tree.h"
> >  #include "sysemu/qtest.h"
> >
> > +#include <libfdt.h>
> > +
> >  #if defined(TARGET_RISCV32)
> >  # define KERNEL_BOOT_ADDRESS 0x80400000
> >  #else
> > @@ -155,3 +159,45 @@ hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> >
> >      return *start + size;
> >  }
> > +
> > +void riscv_setup_rom_reset_vec(hwaddr start_addr, hwaddr rom_base,
> > +                               hwaddr rom_size, 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) */
> > +        0xf1402573,                  /*     csrr   a0, mhartid  */
> > +#if defined(TARGET_RISCV32)
> > +        0x0182a283,                  /*     lw     t0, 24(t0) */
> > +#elif defined(TARGET_RISCV64)
> > +        0x0182b283,                  /*     ld     t0, 24(t0) */
> > +#endif
> > +        0x00028067,                  /*     jr     t0 */
> > +        0x00000000,
> > +        start_addr,                  /* start: .dword */
> > +        0x00000000,
> > +                                     /* dtb: */
> > +    };
> > +
> > +    /* copy in the reset vector in little_endian byte order */
> > +    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > +        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > +    }
> Maybe use ARRAY_SIZE(reset_vec) instead of sizeof(reset_vec) >> 2 ?
>

Yeah. That's better. Thanks.

> > +    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 f9fef2be9170..c2712570e0d9 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -325,7 +325,6 @@ static void sifive_u_machine_init(MachineState *machine)
> >      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >      MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> >      target_ulong start_addr = memmap[SIFIVE_U_DRAM].base;
> > -    int i;
> >
> >      /* Initialize SoC */
> >      object_initialize_child(OBJECT(machine), "soc", &s->soc,
> > @@ -374,40 +373,9 @@ static void sifive_u_machine_init(MachineState *machine)
> >          start_addr = memmap[SIFIVE_U_FLASH0].base;
> >      }
> >
> > -    /* reset vector */
> > -    uint32_t reset_vec[8] = {
> > -        0x00000297,                    /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > -        0x02028593,                    /*     addi   a1, t0, %pcrel_lo(1b) */
> > -        0xf1402573,                    /*     csrr   a0, mhartid  */
> > -#if defined(TARGET_RISCV32)
> > -        0x0182a283,                    /*     lw     t0, 24(t0) */
> > -#elif defined(TARGET_RISCV64)
> > -        0x0182b283,                    /*     ld     t0, 24(t0) */
> > -#endif
> > -        0x00028067,                    /*     jr     t0 */
> > -        0x00000000,
> > -        start_addr,                    /* start: .dword */
> > -        0x00000000,
> > -                                       /* dtb: */
> > -    };
> > -
> > -    /* copy in the reset vector in little_endian byte order */
> > -    for (i = 0; i < sizeof(reset_vec) >> 2; 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);
> > +    /* load the reset vector */
> > +    riscv_setup_rom_reset_vec(start_addr, memmap[SIFIVE_U_MROM].base,
> > +                              memmap[SIFIVE_U_MROM].size, s->fdt);
> >  }
> >
> >  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 7bbbdb50363d..238eae48716a 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -165,7 +165,6 @@ static void spike_board_init(MachineState *machine)
> >      MemoryRegion *system_memory = get_system_memory();
> >      MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > -    int i;
> >      unsigned int smp_cpus = machine->smp.cpus;
> >
> >      /* Initialize SOC */
> > @@ -213,40 +212,9 @@ static void spike_board_init(MachineState *machine)
> >          }
> >      }
> >
> > -    /* reset vector */
> > -    uint32_t reset_vec[8] = {
> > -        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > -        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > -        0xf1402573,                  /*     csrr   a0, mhartid  */
> > -#if defined(TARGET_RISCV32)
> > -        0x0182a283,                  /*     lw     t0, 24(t0) */
> > -#elif defined(TARGET_RISCV64)
> > -        0x0182b283,                  /*     ld     t0, 24(t0) */
> > -#endif
> > -        0x00028067,                  /*     jr     t0 */
> > -        0x00000000,
> > -        memmap[SPIKE_DRAM].base,     /* start: .dword DRAM_BASE */
> > -        0x00000000,
> > -                                     /* dtb: */
> > -    };
> > -
> > -    /* copy in the reset vector in little_endian byte order */
> > -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > -        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > -    }
> > -    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > -                          memmap[SPIKE_MROM].base, &address_space_memory);
> > -
> > -    /* copy in the device tree */
> > -    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > -            memmap[SPIKE_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[SPIKE_MROM].base + sizeof(reset_vec),
> > -                          &address_space_memory);
> > +    /* load the reset vector */
> > +    riscv_setup_rom_reset_vec(memmap[SPIKE_DRAM].base, memmap[SPIKE_MROM].base,
> > +                              memmap[SPIKE_MROM].size, 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 4e4c494a7050..a8e2d58cc067 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -536,40 +536,9 @@ static void virt_machine_init(MachineState *machine)
> >          start_addr = virt_memmap[VIRT_FLASH].base;
> >      }
> >
> > -    /* reset vector */
> > -    uint32_t reset_vec[8] = {
> > -        0x00000297,                  /* 1:  auipc  t0, %pcrel_hi(dtb) */
> > -        0x02028593,                  /*     addi   a1, t0, %pcrel_lo(1b) */
> > -        0xf1402573,                  /*     csrr   a0, mhartid  */
> > -#if defined(TARGET_RISCV32)
> > -        0x0182a283,                  /*     lw     t0, 24(t0) */
> > -#elif defined(TARGET_RISCV64)
> > -        0x0182b283,                  /*     ld     t0, 24(t0) */
> > -#endif
> > -        0x00028067,                  /*     jr     t0 */
> > -        0x00000000,
> > -        start_addr,                  /* start: .dword */
> > -        0x00000000,
> > -                                     /* dtb: */
> > -    };
> > -
> > -    /* copy in the reset vector in little_endian byte order */
> > -    for (i = 0; i < sizeof(reset_vec) >> 2; i++) {
> > -        reset_vec[i] = cpu_to_le32(reset_vec[i]);
> > -    }
> > -    rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
> > -                          memmap[VIRT_MROM].base, &address_space_memory);
> > -
> > -    /* copy in the device tree */
> > -    if (fdt_pack(s->fdt) || fdt_totalsize(s->fdt) >
> > -            memmap[VIRT_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[VIRT_MROM].base + sizeof(reset_vec),
> > -                          &address_space_memory);
> > +    /* load the reset vector */
> > +    riscv_setup_rom_reset_vec(start_addr, virt_memmap[VIRT_MROM].base,
> > +                              virt_memmap[VIRT_MROM].size, 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 9daa98da08d7..3e9759c89aa2 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -35,5 +35,7 @@ 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);
> > +void riscv_setup_rom_reset_vec(hwaddr saddr, hwaddr rom_base,
> > +                               hwaddr rom_size, void *fdt);
> >
> >  #endif /* RISCV_BOOT_H */
> > --
> > 2.26.2
> >
> >
>


-- 
Regards,
Atish