* [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-15 23:00 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-15 23:00 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: bmeng.cn, palmer, alistair.francis, alistair23
We were accidently passing RISCVHartArrayState by value instead of
pointer. The type is 824 bytes long so let's correct that and pass it by
pointer instead.
Fixes: Coverity CID 1438099
Fixes: Coverity CID 1438100
Fixes: Coverity CID 1438101
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
include/hw/riscv/boot.h | 6 +++---
hw/riscv/boot.c | 8 ++++----
hw/riscv/sifive_u.c | 10 +++++-----
hw/riscv/spike.c | 8 ++++----
hw/riscv/virt.c | 8 ++++----
5 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 20ff5fe5e5..11a21dd584 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -24,9 +24,9 @@
#include "hw/loader.h"
#include "hw/riscv/riscv_hart.h"
-bool riscv_is_32bit(RISCVHartArrayState harts);
+bool riscv_is_32bit(RISCVHartArrayState *harts);
-target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
+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,
@@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
+void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
hwaddr saddr,
hwaddr rom_base, hwaddr rom_size,
uint64_t kernel_entry,
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 83586aef41..acf77675b2 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -33,14 +33,14 @@
#include <libfdt.h>
-bool riscv_is_32bit(RISCVHartArrayState harts)
+bool riscv_is_32bit(RISCVHartArrayState *harts)
{
- RISCVCPU hart = harts.harts[0];
+ RISCVCPU hart = harts->harts[0];
return riscv_cpu_is_32bit(&hart.env);
}
-target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
+target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
target_ulong firmware_end_addr) {
if (riscv_is_32bit(harts)) {
return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
@@ -247,7 +247,7 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
&address_space_memory);
}
-void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
+void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
hwaddr start_addr,
hwaddr rom_base, hwaddr rom_size,
uint64_t kernel_entry,
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index f5c400dd44..d23f349b4e 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_32bit(s->soc.u_cpus));
+ 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_32bit(s->soc.u_cpus)) {
+ 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(s->soc.u_cpus,
+ 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_32bit(s->soc.u_cpus)) {
+ 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_32bit(s->soc.u_cpus)) {
+ 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 e723ca0ac9..56986ecfe0 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_32bit(s->soc[0]));
+ 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_32bit(s->soc[0])) {
+ 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(s->soc[0],
+ 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, s->soc[0], 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 8de4c35c9d..2299b3a6be 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_32bit(s->soc[0]));
+ 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_32bit(s->soc[0])) {
+ 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(s->soc[0],
+ 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,7 @@ 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, s->soc[0], start_addr,
+ 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);
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-15 23:00 ` Alistair Francis
@ 2021-01-15 23:03 ` Palmer Dabbelt
-1 siblings, 0 replies; 20+ messages in thread
From: Palmer Dabbelt @ 2021-01-15 23:03 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Fri, 15 Jan 2021 15:00:27 PST (-0800), Alistair Francis wrote:
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
>
> return riscv_cpu_is_32bit(&hart.env);
> }
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
> target_ulong firmware_end_addr) {
> if (riscv_is_32bit(harts)) {
> return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> @@ -247,7 +247,7 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> &address_space_memory);
> }
>
> -void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr start_addr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index f5c400dd44..d23f349b4e 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_32bit(s->soc.u_cpus));
> + 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_32bit(s->soc.u_cpus)) {
> + 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(s->soc.u_cpus,
> + 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_32bit(s->soc.u_cpus)) {
> + 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_32bit(s->soc.u_cpus)) {
> + 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 e723ca0ac9..56986ecfe0 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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, s->soc[0], 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 8de4c35c9d..2299b3a6be 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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,7 @@ 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, s->soc[0], start_addr,
> + 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);
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-15 23:03 ` Palmer Dabbelt
0 siblings, 0 replies; 20+ messages in thread
From: Palmer Dabbelt @ 2021-01-15 23:03 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, qemu-riscv, bmeng.cn, Alistair Francis, alistair23
On Fri, 15 Jan 2021 15:00:27 PST (-0800), Alistair Francis wrote:
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
>
> return riscv_cpu_is_32bit(&hart.env);
> }
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
> target_ulong firmware_end_addr) {
> if (riscv_is_32bit(harts)) {
> return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> @@ -247,7 +247,7 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> &address_space_memory);
> }
>
> -void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr start_addr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index f5c400dd44..d23f349b4e 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_32bit(s->soc.u_cpus));
> + 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_32bit(s->soc.u_cpus)) {
> + 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(s->soc.u_cpus,
> + 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_32bit(s->soc.u_cpus)) {
> + 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_32bit(s->soc.u_cpus)) {
> + 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 e723ca0ac9..56986ecfe0 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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, s->soc[0], 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 8de4c35c9d..2299b3a6be 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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,7 @@ 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, s->soc[0], start_addr,
> + 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);
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-15 23:00 ` Alistair Francis
@ 2021-01-16 16:30 ` Bin Meng
-1 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2021-01-16 16:30 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Sat, Jan 16, 2021 at 7:00 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
Where can I look at the Coverity report for QEMU?
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-16 16:30 ` Bin Meng
0 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2021-01-16 16:30 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Palmer Dabbelt, Alistair Francis
On Sat, Jan 16, 2021 at 7:00 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
Where can I look at the Coverity report for QEMU?
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-16 16:30 ` Bin Meng
@ 2021-01-16 17:50 ` Alistair Francis
-1 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 17:50 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Sat, Jan 16, 2021 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Jan 16, 2021 at 7:00 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > We were accidently passing RISCVHartArrayState by value instead of
> > pointer. The type is 824 bytes long so let's correct that and pass it by
> > pointer instead.
> >
> > Fixes: Coverity CID 1438099
> > Fixes: Coverity CID 1438100
> > Fixes: Coverity CID 1438101
>
> Where can I look at the Coverity report for QEMU?
I don't think you can. I think there are only a few people who can see
them and they just report them to everyone else.
>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/riscv/boot.h | 6 +++---
> > hw/riscv/boot.c | 8 ++++----
> > hw/riscv/sifive_u.c | 10 +++++-----
> > hw/riscv/spike.c | 8 ++++----
> > hw/riscv/virt.c | 8 ++++----
> > 5 files changed, 20 insertions(+), 20 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
Thanks!
Alistair
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-16 17:50 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 17:50 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, Palmer Dabbelt
On Sat, Jan 16, 2021 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Jan 16, 2021 at 7:00 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > We were accidently passing RISCVHartArrayState by value instead of
> > pointer. The type is 824 bytes long so let's correct that and pass it by
> > pointer instead.
> >
> > Fixes: Coverity CID 1438099
> > Fixes: Coverity CID 1438100
> > Fixes: Coverity CID 1438101
>
> Where can I look at the Coverity report for QEMU?
I don't think you can. I think there are only a few people who can see
them and they just report them to everyone else.
>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/riscv/boot.h | 6 +++---
> > hw/riscv/boot.c | 8 ++++----
> > hw/riscv/sifive_u.c | 10 +++++-----
> > hw/riscv/spike.c | 8 ++++----
> > hw/riscv/virt.c | 8 ++++----
> > 5 files changed, 20 insertions(+), 20 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
Thanks!
Alistair
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-15 23:00 ` Alistair Francis
@ 2021-01-16 18:55 ` Alistair Francis
-1 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 18:55 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, Bin Meng, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Fri, Jan 15, 2021 at 3:00 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
>
> return riscv_cpu_is_32bit(&hart.env);
> }
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
> target_ulong firmware_end_addr) {
> if (riscv_is_32bit(harts)) {
> return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> @@ -247,7 +247,7 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> &address_space_memory);
> }
>
> -void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr start_addr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index f5c400dd44..d23f349b4e 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_32bit(s->soc.u_cpus));
> + 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_32bit(s->soc.u_cpus)) {
> + 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(s->soc.u_cpus,
> + 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_32bit(s->soc.u_cpus)) {
> + 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_32bit(s->soc.u_cpus)) {
> + 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 e723ca0ac9..56986ecfe0 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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, s->soc[0], 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 8de4c35c9d..2299b3a6be 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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,7 @@ 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, s->soc[0], start_addr,
> + 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);
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-16 18:55 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 18:55 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V, Bin Meng,
Palmer Dabbelt
On Fri, Jan 15, 2021 at 3:00 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
>
> return riscv_cpu_is_32bit(&hart.env);
> }
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState *harts,
> target_ulong firmware_end_addr) {
> if (riscv_is_32bit(harts)) {
> return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> @@ -247,7 +247,7 @@ void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> &address_space_memory);
> }
>
> -void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr start_addr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index f5c400dd44..d23f349b4e 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_32bit(s->soc.u_cpus));
> + 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_32bit(s->soc.u_cpus)) {
> + 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(s->soc.u_cpus,
> + 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_32bit(s->soc.u_cpus)) {
> + 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_32bit(s->soc.u_cpus)) {
> + 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 e723ca0ac9..56986ecfe0 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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, s->soc[0], 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 8de4c35c9d..2299b3a6be 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_32bit(s->soc[0]));
> + 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_32bit(s->soc[0])) {
> + 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(s->soc[0],
> + 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,7 @@ 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, s->soc[0], start_addr,
> + 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);
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-15 23:00 ` Alistair Francis
@ 2021-01-16 22:32 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-16 22:32 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 1/16/21 12:00 AM, Alistair Francis wrote:
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
This doesn't look improved. Maybe you want:
return riscv_cpu_is_32bit(&harts->harts[0].env);
>
> return riscv_cpu_is_32bit(&hart.env);
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-16 22:32 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-16 22:32 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: bmeng.cn, palmer, alistair23
On 1/16/21 12:00 AM, Alistair Francis wrote:
> We were accidently passing RISCVHartArrayState by value instead of
> pointer. The type is 824 bytes long so let's correct that and pass it by
> pointer instead.
>
> Fixes: Coverity CID 1438099
> Fixes: Coverity CID 1438100
> Fixes: Coverity CID 1438101
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 6 +++---
> hw/riscv/boot.c | 8 ++++----
> hw/riscv/sifive_u.c | 10 +++++-----
> hw/riscv/spike.c | 8 ++++----
> hw/riscv/virt.c | 8 ++++----
> 5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 20ff5fe5e5..11a21dd584 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -24,9 +24,9 @@
> #include "hw/loader.h"
> #include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32bit(RISCVHartArrayState harts);
> +bool riscv_is_32bit(RISCVHartArrayState *harts);
>
> -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> +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,
> @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 83586aef41..acf77675b2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,14 +33,14 @@
>
> #include <libfdt.h>
>
> -bool riscv_is_32bit(RISCVHartArrayState harts)
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
This doesn't look improved. Maybe you want:
return riscv_cpu_is_32bit(&harts->harts[0].env);
>
> return riscv_cpu_is_32bit(&hart.env);
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-16 22:32 ` Philippe Mathieu-Daudé
@ 2021-01-16 22:38 ` Alistair Francis
-1 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 22:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, Palmer Dabbelt, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers
On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/16/21 12:00 AM, Alistair Francis wrote:
> > We were accidently passing RISCVHartArrayState by value instead of
> > pointer. The type is 824 bytes long so let's correct that and pass it by
> > pointer instead.
> >
> > Fixes: Coverity CID 1438099
> > Fixes: Coverity CID 1438100
> > Fixes: Coverity CID 1438101
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/riscv/boot.h | 6 +++---
> > hw/riscv/boot.c | 8 ++++----
> > hw/riscv/sifive_u.c | 10 +++++-----
> > hw/riscv/spike.c | 8 ++++----
> > hw/riscv/virt.c | 8 ++++----
> > 5 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 20ff5fe5e5..11a21dd584 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -24,9 +24,9 @@
> > #include "hw/loader.h"
> > #include "hw/riscv/riscv_hart.h"
> >
> > -bool riscv_is_32bit(RISCVHartArrayState harts);
> > +bool riscv_is_32bit(RISCVHartArrayState *harts);
> >
> > -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> > +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,
> > @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> > +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> > hwaddr saddr,
> > hwaddr rom_base, hwaddr rom_size,
> > uint64_t kernel_entry,
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 83586aef41..acf77675b2 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -33,14 +33,14 @@
> >
> > #include <libfdt.h>
> >
> > -bool riscv_is_32bit(RISCVHartArrayState harts)
> > +bool riscv_is_32bit(RISCVHartArrayState *harts)
> > {
> > - RISCVCPU hart = harts.harts[0];
> > + RISCVCPU hart = harts->harts[0];
>
> This doesn't look improved. Maybe you want:
>
> return riscv_cpu_is_32bit(&harts->harts[0].env);
I suspect this ends up generating the same code.
Either way, good point I have just squashed this change into the patch.
Alistair
>
> >
> > return riscv_cpu_is_32bit(&hart.env);
> > }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-16 22:38 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2021-01-16 22:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, Bin Meng, Palmer Dabbelt
On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/16/21 12:00 AM, Alistair Francis wrote:
> > We were accidently passing RISCVHartArrayState by value instead of
> > pointer. The type is 824 bytes long so let's correct that and pass it by
> > pointer instead.
> >
> > Fixes: Coverity CID 1438099
> > Fixes: Coverity CID 1438100
> > Fixes: Coverity CID 1438101
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/riscv/boot.h | 6 +++---
> > hw/riscv/boot.c | 8 ++++----
> > hw/riscv/sifive_u.c | 10 +++++-----
> > hw/riscv/spike.c | 8 ++++----
> > hw/riscv/virt.c | 8 ++++----
> > 5 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 20ff5fe5e5..11a21dd584 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -24,9 +24,9 @@
> > #include "hw/loader.h"
> > #include "hw/riscv/riscv_hart.h"
> >
> > -bool riscv_is_32bit(RISCVHartArrayState harts);
> > +bool riscv_is_32bit(RISCVHartArrayState *harts);
> >
> > -target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
> > +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,
> > @@ -42,7 +42,7 @@ 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, RISCVHartArrayState harts,
> > +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> > hwaddr saddr,
> > hwaddr rom_base, hwaddr rom_size,
> > uint64_t kernel_entry,
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 83586aef41..acf77675b2 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -33,14 +33,14 @@
> >
> > #include <libfdt.h>
> >
> > -bool riscv_is_32bit(RISCVHartArrayState harts)
> > +bool riscv_is_32bit(RISCVHartArrayState *harts)
> > {
> > - RISCVCPU hart = harts.harts[0];
> > + RISCVCPU hart = harts->harts[0];
>
> This doesn't look improved. Maybe you want:
>
> return riscv_cpu_is_32bit(&harts->harts[0].env);
I suspect this ends up generating the same code.
Either way, good point I have just squashed this change into the patch.
Alistair
>
> >
> > return riscv_cpu_is_32bit(&hart.env);
> > }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-16 22:38 ` Alistair Francis
@ 2021-01-17 16:52 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-17 16:52 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Palmer Dabbelt, Alistair Francis, Bin Meng, Richard Henderson
On 1/16/21 11:38 PM, Alistair Francis wrote:
> On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 1/16/21 12:00 AM, Alistair Francis wrote:
>>> We were accidently passing RISCVHartArrayState by value instead of
>>> pointer. The type is 824 bytes long so let's correct that and pass it by
>>> pointer instead.
>>>
>>> Fixes: Coverity CID 1438099
>>> Fixes: Coverity CID 1438100
>>> Fixes: Coverity CID 1438101
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> include/hw/riscv/boot.h | 6 +++---
>>> hw/riscv/boot.c | 8 ++++----
>>> hw/riscv/sifive_u.c | 10 +++++-----
>>> hw/riscv/spike.c | 8 ++++----
>>> hw/riscv/virt.c | 8 ++++----
>>> 5 files changed, 20 insertions(+), 20 deletions(-)
...
>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>> index 83586aef41..acf77675b2 100644
>>> --- a/hw/riscv/boot.c
>>> +++ b/hw/riscv/boot.c
>>> @@ -33,14 +33,14 @@
>>>
>>> #include <libfdt.h>
>>>
>>> -bool riscv_is_32bit(RISCVHartArrayState harts)
>>> +bool riscv_is_32bit(RISCVHartArrayState *harts)
>>> {
>>> - RISCVCPU hart = harts.harts[0];
>>> + RISCVCPU hart = harts->harts[0];
>>
>> This doesn't look improved. Maybe you want:
>>
>> return riscv_cpu_is_32bit(&harts->harts[0].env);
>
> I suspect this ends up generating the same code.
If the compiler is smart enough, but I'm not sure it can figure out
only 1 element from the structure is accessed...
My understanding is "first copy the content pointed at '*harts' in
'hart' on the stack", then only use "env".
Cc'ing Eric/Richard to double check.
>
> Either way, good point I have just squashed this change into the patch.
Thanks,
Phil.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-17 16:52 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-17 16:52 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers, Eric Blake, Richard Henderson
On 1/16/21 11:38 PM, Alistair Francis wrote:
> On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 1/16/21 12:00 AM, Alistair Francis wrote:
>>> We were accidently passing RISCVHartArrayState by value instead of
>>> pointer. The type is 824 bytes long so let's correct that and pass it by
>>> pointer instead.
>>>
>>> Fixes: Coverity CID 1438099
>>> Fixes: Coverity CID 1438100
>>> Fixes: Coverity CID 1438101
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> include/hw/riscv/boot.h | 6 +++---
>>> hw/riscv/boot.c | 8 ++++----
>>> hw/riscv/sifive_u.c | 10 +++++-----
>>> hw/riscv/spike.c | 8 ++++----
>>> hw/riscv/virt.c | 8 ++++----
>>> 5 files changed, 20 insertions(+), 20 deletions(-)
...
>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>> index 83586aef41..acf77675b2 100644
>>> --- a/hw/riscv/boot.c
>>> +++ b/hw/riscv/boot.c
>>> @@ -33,14 +33,14 @@
>>>
>>> #include <libfdt.h>
>>>
>>> -bool riscv_is_32bit(RISCVHartArrayState harts)
>>> +bool riscv_is_32bit(RISCVHartArrayState *harts)
>>> {
>>> - RISCVCPU hart = harts.harts[0];
>>> + RISCVCPU hart = harts->harts[0];
>>
>> This doesn't look improved. Maybe you want:
>>
>> return riscv_cpu_is_32bit(&harts->harts[0].env);
>
> I suspect this ends up generating the same code.
If the compiler is smart enough, but I'm not sure it can figure out
only 1 element from the structure is accessed...
My understanding is "first copy the content pointed at '*harts' in
'hart' on the stack", then only use "env".
Cc'ing Eric/Richard to double check.
>
> Either way, good point I have just squashed this change into the patch.
Thanks,
Phil.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-17 16:52 ` Philippe Mathieu-Daudé
@ 2021-01-19 21:50 ` Eric Blake
-1 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2021-01-19 21:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Palmer Dabbelt, Alistair Francis, Bin Meng, Richard Henderson
On 1/17/21 10:52 AM, Philippe Mathieu-Daudé wrote:
> On 1/16/21 11:38 PM, Alistair Francis wrote:
>> On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> On 1/16/21 12:00 AM, Alistair Francis wrote:
>>>> We were accidently passing RISCVHartArrayState by value instead of
accidentally
>>>> pointer. The type is 824 bytes long so let's correct that and pass it by
>>>> pointer instead.
>>>> -bool riscv_is_32bit(RISCVHartArrayState harts)
>>>> +bool riscv_is_32bit(RISCVHartArrayState *harts)
Definitely better,
>>>> {
>>>> - RISCVCPU hart = harts.harts[0];
>>>> + RISCVCPU hart = harts->harts[0];
but yeah, this still results in a copy (unless the compiler optimizes it).
>>>
>>> This doesn't look improved. Maybe you want:
>>>
>>> return riscv_cpu_is_32bit(&harts->harts[0].env);
Whereas this is obviously a pointer into the original without relying on
the compiler to elide a copy.
>>
>> I suspect this ends up generating the same code.
>
> If the compiler is smart enough, but I'm not sure it can figure out
> only 1 element from the structure is accessed...
> My understanding is "first copy the content pointed at '*harts' in
> 'hart' on the stack", then only use "env".
>
> Cc'ing Eric/Richard to double check.
I agree that relying on the compiler optimization is not as
straightforward as writing the code to directly access the correct
pointer from the get-go.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-19 21:50 ` Eric Blake
0 siblings, 0 replies; 20+ messages in thread
From: Eric Blake @ 2021-01-19 21:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers, Richard Henderson
On 1/17/21 10:52 AM, Philippe Mathieu-Daudé wrote:
> On 1/16/21 11:38 PM, Alistair Francis wrote:
>> On Sat, Jan 16, 2021 at 2:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> On 1/16/21 12:00 AM, Alistair Francis wrote:
>>>> We were accidently passing RISCVHartArrayState by value instead of
accidentally
>>>> pointer. The type is 824 bytes long so let's correct that and pass it by
>>>> pointer instead.
>>>> -bool riscv_is_32bit(RISCVHartArrayState harts)
>>>> +bool riscv_is_32bit(RISCVHartArrayState *harts)
Definitely better,
>>>> {
>>>> - RISCVCPU hart = harts.harts[0];
>>>> + RISCVCPU hart = harts->harts[0];
but yeah, this still results in a copy (unless the compiler optimizes it).
>>>
>>> This doesn't look improved. Maybe you want:
>>>
>>> return riscv_cpu_is_32bit(&harts->harts[0].env);
Whereas this is obviously a pointer into the original without relying on
the compiler to elide a copy.
>>
>> I suspect this ends up generating the same code.
>
> If the compiler is smart enough, but I'm not sure it can figure out
> only 1 element from the structure is accessed...
> My understanding is "first copy the content pointed at '*harts' in
> 'hart' on the stack", then only use "env".
>
> Cc'ing Eric/Richard to double check.
I agree that relying on the compiler optimization is not as
straightforward as writing the code to directly access the correct
pointer from the get-go.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
2021-01-15 23:00 ` Alistair Francis
@ 2021-01-18 17:14 ` Richard Henderson
-1 siblings, 0 replies; 20+ messages in thread
From: Richard Henderson @ 2021-01-18 17:14 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 1/15/21 1:00 PM, Alistair Francis wrote:
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
This is a large structure copy as well.
> return riscv_cpu_is_32bit(&hart.env);
> }
Better as
&harts->harts[0].env
surely.
r~
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
@ 2021-01-18 17:14 ` Richard Henderson
0 siblings, 0 replies; 20+ messages in thread
From: Richard Henderson @ 2021-01-18 17:14 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: bmeng.cn, palmer, alistair23
On 1/15/21 1:00 PM, Alistair Francis wrote:
> +bool riscv_is_32bit(RISCVHartArrayState *harts)
> {
> - RISCVCPU hart = harts.harts[0];
> + RISCVCPU hart = harts->harts[0];
This is a large structure copy as well.
> return riscv_cpu_is_32bit(&hart.env);
> }
Better as
&harts->harts[0].env
surely.
r~
^ permalink raw reply [flat|nested] 20+ messages in thread