| 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, }, };
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> --- hw/arm/fby35.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)