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

[Cédric Le Goater]

On 4/4/23 17:54, Peter Maydell wrote:
> 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 :-)

hey :) Indeed.

I have recently taken another approach in the aspeed machines, which is
to introduce a spi_boot region since it is part of the SoC address space.
There is also an extra container to overlap a rom region to speed up boot
because the "execute-in-place mode" is a bit slow (~30 s to reach FW).
So the aspeed code is different now :

   if (!bmc->mmio_exec) {
        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);

        if (mtd0) {
            uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
            aspeed_install_boot_rom(&bmc->soc, blk_by_legacy_dinfo(mtd0),
                                    rom_size);
        }
    }

and we should report the same code in fby35 (which was a copy paste)

The benefits are that we will (soon) be able to create SPI flash devices
from the command line, such as :

        -blockdev node-name=fmc0,driver=file,filename=./flash.img \
        -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 \

and boot from them. That said, the memory leak is still there in
aspeed_install_boot_rom() :

    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);

    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
                           &error_abort);
    memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
                                        boot_rom, 1);
    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);

and coverity didn't catch it ? Anyhow we can use a MemoryRegion under the
machine state to remove the warning like you suggested. I will add that on
the 8.1 TODO list.

Thanks,

C.