On Sat, Jul 02, 2022 at 12:36:46AM -0700, Peter Delevoryas wrote: > On Sat, Jul 02, 2022 at 12:01:48AM -0700, Peter Delevoryas wrote: > > On Sat, Jul 02, 2022 at 08:01:14AM +0200, Cédric Le Goater wrote: > > > On 7/1/22 20:06, Peter Delevoryas wrote: > > > > To support multiple SoC's running simultaneously, we need a unique name > > > > for > > > > each RAM region. DRAM is created by the machine, but SRAM is created by > > > > the > > > > SoC, since in hardware it is part of the SoC's internals. > > > > > > > > We need a way to uniquely identify each SRAM region though, for VM > > > > migration. Since each of the SoC's CPU's has an index which identifies > > > > it > > > > uniquely from other CPU's in the machine, we can use the index of any > > > > of the > > > > CPU's in the SoC to uniquely identify differentiate the SRAM name from > > > > other > > > > SoC SRAM's. In this change, I just elected to use the index of the > > > > first CPU > > > > in each SoC. > > > > > > hopefully the index is allocated. Did you check ? > > > > You mean the CpuState.cpu_index? I think it's allocated at this point, I > > actually had to do some debugging just to get it working cause I typo'd the > > CPU(...) cast at first. I also tried it with the multi-SoC board in your > > aspeed-7.1 branch: > > > > (qemu) qom-get /machine/bmc aspeed.sram.0[0] > > "/machine/bmc/aspeed.sram.0[0]" > > (qemu) qom-get /machine/unattached aspeed.sram.2[0] > > "/machine/unattached/aspeed.sram.2[0]" > > > > I think the SRAM in the ast1030 is initialized without a parent object > > (memory_region_init_ram(..., NULL, ...)) so that's why it's in the > > unattached > > area. But we could fix that, maybe I should send a v2 with that too? > > > > > > > > > > > > Signed-off-by: Peter Delevoryas <m...@pjd.dev> > > > > --- > > > > hw/arm/aspeed_ast10x0.c | 5 ++++- > > > > hw/arm/aspeed_ast2600.c | 5 +++-- > > > > hw/arm/aspeed_soc.c | 5 +++-- > > > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c > > > > index 33ef331771..b6b6f0d053 100644 > > > > --- a/hw/arm/aspeed_ast10x0.c > > > > +++ b/hw/arm/aspeed_ast10x0.c > > > > @@ -159,6 +159,7 @@ static void aspeed_soc_ast1030_realize(DeviceState > > > > *dev_soc, Error **errp) > > > > DeviceState *armv7m; > > > > Error *err = NULL; > > > > int i; > > > > + char name[64]; > > > > if (!clock_has_source(s->sysclk)) { > > > > error_setg(errp, "sysclk clock must be wired up by the board > > > > code"); > > > > @@ -183,7 +184,9 @@ static void aspeed_soc_ast1030_realize(DeviceState > > > > *dev_soc, Error **errp) > > > > sysbus_realize(SYS_BUS_DEVICE(&s->armv7m), &error_abort); > > > > /* Internal SRAM */ > > > > - memory_region_init_ram(&s->sram, NULL, "aspeed.sram", > > > > sc->sram_size, &err); > > > > + snprintf(name, sizeof(name), "aspeed.sram.%d", > > > > + CPU(s->armv7m.cpu)->cpu_index); > > > > + memory_region_init_ram(&s->sram, NULL, name, sc->sram_size, &err); > > > > if (err != NULL) { > > > > error_propagate(errp, err); > > > > return; > > > > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > > > > index 3f0611ac11..7efb9f888a 100644 > > > > --- a/hw/arm/aspeed_ast2600.c > > > > +++ b/hw/arm/aspeed_ast2600.c > > > > @@ -276,6 +276,7 @@ static void aspeed_soc_ast2600_realize(DeviceState > > > > *dev, Error **errp) > > > > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > > > > Error *err = NULL; > > > > qemu_irq irq; > > > > + char name[64]; > > > > > > May be ? > > > > > > g_autofree char *sram_name = > > > g_strdup_printf("aspeed.sram.%d", CPU(&s->cpu[0])->cpu_index); > > > > Hmmm yeah sure why not, I can fix the unattached AST1030 SRAM too. I always > > wanted to use g_autofree some day hehe. > > Actually, can't do this: cpu_index is _not_ initialized at this point (the > start > of the function). armv7m needs to be realized first in the ast1030, cpu[]'s > need > to be realized in other SoC's. I don't think it would be preferable to move > the > autofree statement lower because the convention is to put declarations at the > start of the enclosing block, let me know if you have another idea though.
Disregard this comment I made, we can use autofree here, we should just initialize the pointer later in the function. I was curious if __attribute__((cleanup)) handles this properly, and it does, based on the macOS "leaks" leak detector: Without g_autofree: $ leaks --atExit -- ./build/qemu-system-arm -machine fby35 -drive file=fby35.mtd,format=raw,if=mtd -device loader,file=Y35B CL.elf,addr=0,cpu-num=2 -serial pty -serial pty -serial mon:stdio -display none -S char device redirected to /dev/ttys007 (label serial0) char device redirected to /dev/ttys009 (label serial1) qemu-system-arm: warning: Aspeed iBT has no chardev backend QEMU 7.0.50 monitor - type 'help' for more information (qemu) q Process: qemu-system-arm [64263] Path: /Users/USER/*/qemu-system-arm Load Address: 0x10f181000 Identifier: qemu-system-arm Version: ??? Code Type: X86-64 Platform: macOS Parent Process: leaks [64262] Date/Time: 2022-07-02 08:22:43.852 -0700 Launch Time: 2022-07-02 08:22:42.125 -0700 OS Version: macOS 12.4 (21F79) Report Version: 7 Analysis Tool: /usr/bin/leaks Physical footprint: 443.5M Physical footprint (peak): 546.2M ---- leaks Report Version: 4.0 Process 64263: 62375 nodes malloced for 300124 KB Process 64263: 1 leak for 128 total leaked bytes. 1 (128 bytes) ROOT LEAK: 0x600003b8ea00 [128] length: 13 "aspeed.sram.2" With g_autofree: $ leaks --atExit -- ./build/qemu-system-arm -machine fby35 -drive file=fby35.mtd,format=raw,if=mtd -device loader,file=Y35BCL.elf,addr=0,cpu-num=2 -serial pty -serial pty -serial mon:stdio -display none -S char device redirected to /dev/ttys007 (label serial0) char device redirected to /dev/ttys009 (label serial1) qemu-system-arm: warning: Aspeed iBT has no chardev backend QEMU 7.0.50 monitor - type 'help' for more information (qemu) q Process: qemu-system-arm [65015] Path: /Users/USER/*/qemu-system-arm Load Address: 0x10edf7000 Identifier: qemu-system-arm Version: ??? Code Type: X86-64 Platform: macOS Parent Process: leaks [65014] Date/Time: 2022-07-02 08:23:13.748 -0700 Launch Time: 2022-07-02 08:23:12.285 -0700 OS Version: macOS 12.4 (21F79) Report Version: 7 Analysis Tool: /usr/bin/leaks Physical footprint: 438.6M Physical footprint (peak): 547.0M ---- leaks Report Version: 4.0 Process 65015: 62154 nodes malloced for 299904 KB Process 65015: 0 leaks for 0 total leaked bytes. So I'll send a v2 using g_autofree and g_strdup_printf. > > > > > > > > > > > > Thanks, > > > > > > C. > > > > > > > > > > /* IO space */ > > > > aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->iomem), > > > > "aspeed.io", > > > > @@ -335,8 +336,8 @@ static void aspeed_soc_ast2600_realize(DeviceState > > > > *dev, Error **errp) > > > > } > > > > /* SRAM */ > > > > - memory_region_init_ram(&s->sram, OBJECT(dev), "aspeed.sram", > > > > - sc->sram_size, &err); > > > > + snprintf(name, sizeof(name), "aspeed.sram.%d", > > > > CPU(&s->cpu[0])->cpu_index); > > > > + memory_region_init_ram(&s->sram, OBJECT(dev), name, sc->sram_size, > > > > &err); > > > > if (err) { > > > > error_propagate(errp, err); > > > > return; > > > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > > > > index 0f675e7fcd..1ddba33d2a 100644 > > > > --- a/hw/arm/aspeed_soc.c > > > > +++ b/hw/arm/aspeed_soc.c > > > > @@ -239,6 +239,7 @@ static void aspeed_soc_realize(DeviceState *dev, > > > > Error **errp) > > > > AspeedSoCState *s = ASPEED_SOC(dev); > > > > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > > > > Error *err = NULL; > > > > + char name[64]; > > > > /* IO space */ > > > > aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->iomem), > > > > "aspeed.io", > > > > @@ -259,8 +260,8 @@ static void aspeed_soc_realize(DeviceState *dev, > > > > Error **errp) > > > > } > > > > /* SRAM */ > > > > - memory_region_init_ram(&s->sram, OBJECT(dev), "aspeed.sram", > > > > - sc->sram_size, &err); > > > > + snprintf(name, sizeof(name), "aspeed.sram.%d", > > > > CPU(&s->cpu[0])->cpu_index); > > > > + memory_region_init_ram(&s->sram, OBJECT(dev), name, sc->sram_size, > > > > &err); > > > > if (err) { > > > > error_propagate(errp, err); > > > > return; > > >