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