On Sun, Jun 16, 2019 at 1:15 AM Palmer Dabbelt <pal...@sifive.com> wrote:
>
> On Fri, 14 Jun 2019 05:25:50 PDT (-0700), phi...@redhat.com wrote:
> > On 6/14/19 2:08 PM, Palmer Dabbelt wrote:
> >> Coverity pointed out a memory leak in riscv_sifive_e_soc_realize(),
> >> where a pair of recently added MemoryRegion instances would not be freed
> >> if there were errors elsewhere in the function. The fix here is to
> >> simply not use dynamic allocation for these instances: there's always
> >> one of each in SiFiveESoCState, so instead we just include them within
> >> the struct.
> >>
> >> Thanks to Peter for pointing out the bug and suggesting the fix!
> >
> > a.k.a. Suggested-by: Peter Maydell <peter.mayd...@linaro.org>
> >
> > Maybe the thanks can go below the '---' tag, so it doesn't stay in the
> > git history.
>
> Works for me.
With the updated Suggested-by line:
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
>
> >
> >>
> >> Fixes: 30efbf330a45 ("SiFive RISC-V GPIO Device")
> >> Signed-off-by: Palmer Dabbelt <pal...@sifive.com>
> >
> > Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>
> Thanks!
>
> >
> >> ---
> >> hw/riscv/sifive_e.c | 12 +++++-------
> >> include/hw/riscv/sifive_e.h | 2 ++
> >> 2 files changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> >> index 80ac56fa7d5e..83375afcd1d6 100644
> >> --- a/hw/riscv/sifive_e.c
> >> +++ b/hw/riscv/sifive_e.c
> >> @@ -158,17 +158,15 @@ static void riscv_sifive_e_soc_realize(DeviceState
> >> *dev, Error **errp)
> >>
> >> SiFiveESoCState *s = RISCV_E_SOC(dev);
> >> MemoryRegion *sys_mem = get_system_memory();
> >> - MemoryRegion *xip_mem = g_new(MemoryRegion, 1);
> >> - MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >>
> >> object_property_set_bool(OBJECT(&s->cpus), true, "realized",
> >> &error_abort);
> >>
> >> /* Mask ROM */
> >> - memory_region_init_rom(mask_rom, NULL, "riscv.sifive.e.mrom",
> >> + memory_region_init_rom(&s->mask_rom, NULL, "riscv.sifive.e.mrom",
> >> memmap[SIFIVE_E_MROM].size, &error_fatal);
> >> memory_region_add_subregion(sys_mem,
> >> - memmap[SIFIVE_E_MROM].base, mask_rom);
> >> + memmap[SIFIVE_E_MROM].base, &s->mask_rom);
> >>
> >> /* MMIO */
> >> s->plic = sifive_plic_create(memmap[SIFIVE_E_PLIC].base,
> >> @@ -228,10 +226,10 @@ static void riscv_sifive_e_soc_realize(DeviceState
> >> *dev, Error **errp)
> >> memmap[SIFIVE_E_PWM2].base, memmap[SIFIVE_E_PWM2].size);
> >>
> >> /* Flash memory */
> >> - memory_region_init_ram(xip_mem, NULL, "riscv.sifive.e.xip",
> >> + memory_region_init_ram(&s->xip_mem, NULL, "riscv.sifive.e.xip",
> >> memmap[SIFIVE_E_XIP].size, &error_fatal);
> >> - memory_region_set_readonly(xip_mem, true);
> >> - memory_region_add_subregion(sys_mem, memmap[SIFIVE_E_XIP].base,
> >> xip_mem);
> >> + memory_region_set_readonly(&s->xip_mem, true);
> >> + memory_region_add_subregion(sys_mem, memmap[SIFIVE_E_XIP].base,
> >> &s->xip_mem);
> >> }
> >>
> >> static void riscv_sifive_e_machine_init(MachineClass *mc)
> >> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> >> index 3b14eb74621f..d175b24cb209 100644
> >> --- a/include/hw/riscv/sifive_e.h
> >> +++ b/include/hw/riscv/sifive_e.h
> >> @@ -33,6 +33,8 @@ typedef struct SiFiveESoCState {
> >> RISCVHartArrayState cpus;
> >> DeviceState *plic;
> >> SIFIVEGPIOState gpio;
> >> + MemoryRegion xip_mem;
> >> + MemoryRegion mask_rom;
> >> } SiFiveESoCState;
> >>
> >> typedef struct SiFiveEState {
> >>
>
