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