Re: [PATCH] aspeed: Create SRAM name from first CPU index

[Peter Delevoryas]

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 <me@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;
> > >