Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer

From: Alistair Francis <alistair23@gmail.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Bin Meng <bmeng.cn@gmail.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH v1 1/1] riscv: Pass RISCVHartArrayState by pointer
Date: Sat, 16 Jan 2021 14:38:32 -0800	[thread overview]
Message-ID: <CAKmqyKNvtuY2eaQHiM4YMKyy_bWAiHFrMan_R+7M_fw7=ioW=A@mail.gmail.com> (raw)
In-Reply-To: <a566580a-ef30-0d2f-116e-1e3666f1187d@amsat.org>

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);
> >  }