Message ID | 20220714154456.2565189-11-clg@kaod.org |
---|---|
State | New |
Headers | show |
Series | [PULL,01/19] aspeed: sbc: Allow per-machine settings | expand |
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
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.
diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c index 5c5224d37471..e7405f6570b3 100644 --- a/hw/arm/fby35.c +++ b/hw/arm/fby35.c @@ -9,6 +9,7 @@ #include "qemu/units.h" #include "qapi/error.h" #include "sysemu/sysemu.h" +#include "sysemu/block-backend.h" #include "hw/boards.h" #include "hw/arm/aspeed_soc.h" @@ -23,12 +24,49 @@ struct Fby35State { MemoryRegion bmc_boot_rom; AspeedSoCState bmc; + + bool mmio_exec; }; #define FBY35_BMC_RAM_SIZE (2 * GiB) +#define FBY35_BMC_FIRMWARE_ADDR 0x0 + +static void fby35_bmc_write_boot_rom(DriveInfo *dinfo, MemoryRegion *mr, + hwaddr offset, size_t rom_size, + Error **errp) +{ + BlockBackend *blk = blk_by_legacy_dinfo(dinfo); + g_autofree void *storage = NULL; + int64_t size; + + /* + * The block backend size should have already been 'validated' by + * the creation of the m25p80 object. + */ + size = blk_getlength(blk); + if (size <= 0) { + error_setg(errp, "failed to get flash size"); + return; + } + + if (rom_size > size) { + rom_size = size; + } + + storage = g_malloc0(rom_size); + if (blk_pread(blk, 0, rom_size, storage, 0) < 0) { + error_setg(errp, "failed to read the initial flash content"); + return; + } + + /* TODO: find a better way to install the ROM */ + memcpy(memory_region_get_ram_ptr(mr) + offset, storage, rom_size); +} 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); + 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); + } + } } static void fby35_init(MachineState *machine) @@ -57,6 +117,22 @@ static void fby35_init(MachineState *machine) fby35_bmc_init(s); } + +static bool fby35_get_mmio_exec(Object *obj, Error **errp) +{ + return FBY35(obj)->mmio_exec; +} + +static void fby35_set_mmio_exec(Object *obj, bool value, Error **errp) +{ + FBY35(obj)->mmio_exec = value; +} + +static void fby35_instance_init(Object *obj) +{ + FBY35(obj)->mmio_exec = false; +} + static void fby35_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -66,6 +142,12 @@ static void fby35_class_init(ObjectClass *oc, void *data) mc->no_floppy = 1; mc->no_cdrom = 1; mc->min_cpus = mc->max_cpus = mc->default_cpus = 2; + + object_class_property_add_bool(oc, "execute-in-place", + fby35_get_mmio_exec, + fby35_set_mmio_exec); + object_class_property_set_description(oc, "execute-in-place", + "boot directly from CE0 flash device"); } static const TypeInfo fby35_types[] = { @@ -74,6 +156,7 @@ static const TypeInfo fby35_types[] = { .parent = TYPE_MACHINE, .class_init = fby35_class_init, .instance_size = sizeof(Fby35State), + .instance_init = fby35_instance_init, }, };