diff mbox series

[PULL,22/23] hw/riscv: Use the CPU to determine if 32-bit

Message ID 20201218060114.3591217-23-alistair.francis@wdc.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/23] hw/riscv: sifive_u: Add UART1 DT node in the generated DTB | expand

Commit Message

Alistair Francis Dec. 18, 2020, 6:01 a.m. UTC 
Instead of using string compares to determine if a RISC-V machine is
using 32-bit or 64-bit CPUs we can use the initalised CPUs. This avoids
us having to maintain a list of CPU names to compare against.

This commit also fixes the name of the function to match the
riscv_cpu_is_32bit() function.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id: 8ab7614e5df93ab5267788b73dcd75f9f5615e82.1608142916.git.alistair.francis@wdc.com
---
 include/hw/riscv/boot.h |  8 +++++---
 hw/riscv/boot.c         | 31 ++++++++++---------------------
 hw/riscv/sifive_u.c     | 10 +++++-----
 hw/riscv/spike.c        |  8 ++++----
 hw/riscv/virt.c         |  9 +++++----
 5 files changed, 29 insertions(+), 37 deletions(-)

Comments

Peter Maydell Jan. 10, 2021, 7:55 p.m. UTC | #1 
On Fri, 18 Dec 2020 at 06:01, Alistair Francis <alistair.francis@wdc.com> wrote:
>
> Instead of using string compares to determine if a RISC-V machine is
> using 32-bit or 64-bit CPUs we can use the initalised CPUs. This avoids
> us having to maintain a list of CPU names to compare against.
>
> This commit also fixes the name of the function to match the
> riscv_cpu_is_32bit() function.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-id: 8ab7614e5df93ab5267788b73dcd75f9f5615e82.1608142916.git.alistair.francis@wdc.com

Hi; coverity points out a probably-unintentional inefficiency here
(CID 1438099, CID 1438100, CID 1438101):

> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,28 +33,16 @@
>
>  #include <libfdt.h>
>
> -bool riscv_is_32_bit(MachineState *machine)
> +bool riscv_is_32bit(RISCVHartArrayState harts)

The RISCVHartArrayState type is 824 bytes long. That's a very
big type to be passing by value. You probably wanted to pass a
pointer to it instead. Similarly for the arguments to
riscv_calc_kernel_start_addr() and riscv_setup_rom_reset_vec().

thanks
-- PMM
Alistair Francis Jan. 15, 2021, 1:20 a.m. UTC | #2 
On Sun, Jan 10, 2021 at 11:55 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 18 Dec 2020 at 06:01, Alistair Francis <alistair.francis@wdc.com> wrote:
> >
> > Instead of using string compares to determine if a RISC-V machine is
> > using 32-bit or 64-bit CPUs we can use the initalised CPUs. This avoids
> > us having to maintain a list of CPU names to compare against.
> >
> > This commit also fixes the name of the function to match the
> > riscv_cpu_is_32bit() function.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Message-id: 8ab7614e5df93ab5267788b73dcd75f9f5615e82.1608142916.git.alistair.francis@wdc.com
>
> Hi; coverity points out a probably-unintentional inefficiency here
> (CID 1438099, CID 1438100, CID 1438101):
>
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -33,28 +33,16 @@
> >
> >  #include <libfdt.h>
> >
> > -bool riscv_is_32_bit(MachineState *machine)
> > +bool riscv_is_32bit(RISCVHartArrayState harts)
>
> The RISCVHartArrayState type is 824 bytes long. That's a very
> big type to be passing by value. You probably wanted to pass a
> pointer to it instead. Similarly for the arguments to
> riscv_calc_kernel_start_addr() and riscv_setup_rom_reset_vec().

Thanks Peter, I'll send a patch.

Alistair

>
> thanks
> -- PMM
diff mbox series

Patch

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index b6d37a91d6..20ff5fe5e5 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -22,10 +22,11 @@ 
 
 #include "exec/cpu-defs.h"
 #include "hw/loader.h"
+#include "hw/riscv/riscv_hart.h"
 
-bool riscv_is_32_bit(MachineState *machine);
+bool riscv_is_32bit(RISCVHartArrayState harts);
 
-target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
                                           target_ulong firmware_end_addr);
 target_ulong riscv_find_and_load_firmware(MachineState *machine,
                                           const char *default_machine_firmware,
@@ -41,7 +42,8 @@  target_ulong riscv_load_kernel(const char *kernel_filename,
 hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
                          uint64_t kernel_entry, hwaddr *start);
 uint32_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
-void riscv_setup_rom_reset_vec(MachineState *machine, hwaddr saddr,
+void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
+                               hwaddr saddr,
                                hwaddr rom_base, hwaddr rom_size,
                                uint64_t kernel_entry,
                                uint32_t fdt_load_addr, void *fdt);
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 6bce6fb485..83586aef41 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -33,28 +33,16 @@ 
 
 #include <libfdt.h>
 
-bool riscv_is_32_bit(MachineState *machine)
+bool riscv_is_32bit(RISCVHartArrayState harts)
 {
-    /*
-     * To determine if the CPU is 32-bit we need to check a few different CPUs.
-     *
-     * If the CPU starts with rv32
-     * If the CPU is a sifive 3 seriries CPU (E31, U34)
-     * If it's the Ibex CPU
-     */
-    if (!strncmp(machine->cpu_type, "rv32", 4) ||
-        (!strncmp(machine->cpu_type, "sifive", 6) &&
-            machine->cpu_type[8] == '3') ||
-        !strncmp(machine->cpu_type, "lowrisc-ibex", 12)) {
-        return true;
-    } else {
-        return false;
-    }
+    RISCVCPU hart = harts.harts[0];
+
+    return riscv_cpu_is_32bit(&hart.env);
 }
 
-target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
                                           target_ulong firmware_end_addr) {
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(harts)) {
         return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
     } else {
         return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
@@ -259,7 +247,8 @@  void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
                            &address_space_memory);
 }
 
-void riscv_setup_rom_reset_vec(MachineState *machine, hwaddr start_addr,
+void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
+                               hwaddr start_addr,
                                hwaddr rom_base, hwaddr rom_size,
                                uint64_t kernel_entry,
                                uint32_t fdt_load_addr, void *fdt)
@@ -267,7 +256,7 @@  void riscv_setup_rom_reset_vec(MachineState *machine, hwaddr start_addr,
     int i;
     uint32_t start_addr_hi32 = 0x00000000;
 
-    if (!riscv_is_32_bit(machine)) {
+    if (!riscv_is_32bit(harts)) {
         start_addr_hi32 = start_addr >> 32;
     }
     /* reset vector */
@@ -284,7 +273,7 @@  void riscv_setup_rom_reset_vec(MachineState *machine, hwaddr start_addr,
         0x00000000,
                                      /* fw_dyn: */
     };
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(harts)) {
         reset_vec[3] = 0x0202a583;   /*     lw     a1, 32(t0) */
         reset_vec[4] = 0x0182a283;   /*     lw     t0, 24(t0) */
     } else {
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 170e49315f..f5c400dd44 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -466,7 +466,7 @@  static void sifive_u_machine_init(MachineState *machine)
 
     /* create device tree */
     create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32_bit(machine));
+               riscv_is_32bit(s->soc.u_cpus));
 
     if (s->start_in_flash) {
         /*
@@ -495,7 +495,7 @@  static void sifive_u_machine_init(MachineState *machine)
         break;
     }
 
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(s->soc.u_cpus)) {
         firmware_end_addr = riscv_find_and_load_firmware(machine,
                                     "opensbi-riscv32-generic-fw_dynamic.bin",
                                     start_addr, NULL);
@@ -506,7 +506,7 @@  static void sifive_u_machine_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+        kernel_start_addr = riscv_calc_kernel_start_addr(s->soc.u_cpus,
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine->kernel_filename,
@@ -533,7 +533,7 @@  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 (!riscv_is_32_bit(machine)) {
+    if (!riscv_is_32bit(s->soc.u_cpus)) {
         start_addr_hi32 = (uint64_t)start_addr >> 32;
     }
 
@@ -552,7 +552,7 @@  static void sifive_u_machine_init(MachineState *machine)
         0x00000000,
                                        /* fw_dyn: */
     };
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(s->soc.u_cpus)) {
         reset_vec[4] = 0x0202a583;     /*     lw     a1, 32(t0) */
         reset_vec[5] = 0x0182a283;     /*     lw     t0, 24(t0) */
     } else {
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 3e47e4579d..e723ca0ac9 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -244,7 +244,7 @@  static void spike_board_init(MachineState *machine)
 
     /* create device tree */
     create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32_bit(machine));
+               riscv_is_32bit(s->soc[0]));
 
     /* boot rom */
     memory_region_init_rom(mask_rom, NULL, "riscv.spike.mrom",
@@ -257,7 +257,7 @@  static void spike_board_init(MachineState *machine)
      * keeping ELF files here was intentional because BIN files don't work
      * for the Spike machine as HTIF emulation depends on ELF parsing.
      */
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(s->soc[0])) {
         firmware_end_addr = riscv_find_and_load_firmware(machine,
                                     "opensbi-riscv32-generic-fw_dynamic.elf",
                                     memmap[SPIKE_DRAM].base,
@@ -270,7 +270,7 @@  static void spike_board_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+        kernel_start_addr = riscv_calc_kernel_start_addr(s->soc[0],
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine->kernel_filename,
@@ -299,7 +299,7 @@  static void spike_board_init(MachineState *machine)
     fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
                                    machine->ram_size, s->fdt);
     /* load the reset vector */
-    riscv_setup_rom_reset_vec(machine, memmap[SPIKE_DRAM].base,
+    riscv_setup_rom_reset_vec(machine, s->soc[0], memmap[SPIKE_DRAM].base,
                               memmap[SPIKE_MROM].base,
                               memmap[SPIKE_MROM].size, kernel_entry,
                               fdt_load_addr, s->fdt);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 9321d8eda5..8de4c35c9d 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -601,7 +601,7 @@  static void virt_machine_init(MachineState *machine)
 
     /* create device tree */
     create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
-               riscv_is_32_bit(machine));
+               riscv_is_32bit(s->soc[0]));
 
     /* boot rom */
     memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
@@ -609,7 +609,7 @@  static void virt_machine_init(MachineState *machine)
     memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
                                 mask_rom);
 
-    if (riscv_is_32_bit(machine)) {
+    if (riscv_is_32bit(s->soc[0])) {
         firmware_end_addr = riscv_find_and_load_firmware(machine,
                                     "opensbi-riscv32-generic-fw_dynamic.bin",
                                     start_addr, NULL);
@@ -620,7 +620,7 @@  static void virt_machine_init(MachineState *machine)
     }
 
     if (machine->kernel_filename) {
-        kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+        kernel_start_addr = riscv_calc_kernel_start_addr(s->soc[0],
                                                          firmware_end_addr);
 
         kernel_entry = riscv_load_kernel(machine->kernel_filename,
@@ -656,7 +656,8 @@  static void virt_machine_init(MachineState *machine)
     fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
                                    machine->ram_size, s->fdt);
     /* load the reset vector */
-    riscv_setup_rom_reset_vec(machine, start_addr, virt_memmap[VIRT_MROM].base,
+    riscv_setup_rom_reset_vec(machine, s->soc[0], start_addr,
+                              virt_memmap[VIRT_MROM].base,
                               virt_memmap[VIRT_MROM].size, kernel_entry,
                               fdt_load_addr, s->fdt);