| Message ID | f3e04424723e0e222769991896cc82308fd23f76.1610751609.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] riscv: Pass RISCVHartArrayState by pointer | expand |
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>
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>
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
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 >
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); > }
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); > > }
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.
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~
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.
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);
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(-)