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

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

[Peter Delevoryas]

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.

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];
 
     /* 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;
-- 
2.37.0

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

[Cédric Le Goater]

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 ?


> 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);


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;

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

[Peter Delevoryas]

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.

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

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

[Peter Delevoryas]

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.

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

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