Re: [PULL 10/19] aspeed: fby35: Add a bootrom for the BMC

[Peter Maydell]

On Thu, 14 Jul 2022 at 16:45, Cédric Le Goater <clg@kaod.org> wrote:
>
> The BMC boots from the first flash device by fetching instructions
> from the flash contents. Add an alias region on 0x0 for this
> purpose. There are currently performance issues with this method (TBs
> being flushed too often), so as a faster alternative, install the
> flash contents as a ROM in the BMC memory space.
>
> See commit 1a15311a12fa ("hw/arm/aspeed: add a 'execute-in-place'
> property to boot directly from CE0")
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> [ clg: blk_pread() fixes ]
> Message-Id: <20220705191400.41632-8-peter@pjd.dev>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Hi; Coverity has noticed a trivial "memory leak" (CID 1508061) in this code:

>  static void fby35_bmc_init(Fby35State *s)
>  {
> +    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> +
>      memory_region_init(&s->bmc_memory, OBJECT(s), "bmc-memory", UINT64_MAX);
>      memory_region_init_ram(&s->bmc_dram, OBJECT(s), "bmc-dram",
>                             FBY35_BMC_RAM_SIZE, &error_abort);
> @@ -48,6 +86,28 @@ static void fby35_bmc_init(Fby35State *s)
>      qdev_realize(DEVICE(&s->bmc), NULL, &error_abort);
>
>      aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
> +
> +    /* Install first FMC flash content as a boot rom. */
> +    if (drive0) {
> +        AspeedSMCFlash *fl = &s->bmc.fmc.flashes[0];
> +        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);

Here we allocate a new MemoryRegion...

> +        uint64_t size = memory_region_size(&fl->mmio);
> +
> +        if (s->mmio_exec) {
> +            memory_region_init_alias(boot_rom, NULL, "aspeed.boot_rom",
> +                                     &fl->mmio, 0, size);
> +            memory_region_add_subregion(&s->bmc_memory, 
> FBY35_BMC_FIRMWARE_ADDR,
> +                                        boot_rom);
> +        } else {
> +
> +            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
> +                                   size, &error_abort);
> +            memory_region_add_subregion(&s->bmc_memory, 
> FBY35_BMC_FIRMWARE_ADDR,
> +                                        boot_rom);
> +            fby35_bmc_write_boot_rom(drive0, boot_rom, 
> FBY35_BMC_FIRMWARE_ADDR,
> +                                     size, &error_abort);
> +        }

...but we never keep a pointer to it anywhere, so Coverity classes
this as a "memory leak". (It's not really one, because the memory
has to stay live for the whole of QEMU's execution anyway.)

The easy fix is not to allocate a new MR, but instead use
a MemoryRegion field in the Fby35State struct, the way we
do for all the other MRs this function sets up. Conveniently,
there already is a "MemoryRegion bmc_boot_rom" in the struct
which is currently completely unused :-)

thanks
-- PMM