On Thu, 14 Jul 2022 at 16:45, Cédric Le Goater <c...@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 <c...@kaod.org>
> Signed-off-by: Peter Delevoryas <pe...@pjd.dev>
> [ clg: blk_pread() fixes ]
> Message-Id: <20220705191400.41632-8-pe...@pjd.dev>
> Signed-off-by: Cédric Le Goater <c...@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
