* [PATCH] aspeed: Create SRAM name from first CPU index @ 2022-07-01 18:06 Peter Delevoryas 2022-07-02 6:01 ` Cédric Le Goater 0 siblings, 1 reply; 5+ messages in thread From: Peter Delevoryas @ 2022-07-01 18:06 UTC (permalink / raw) Cc: clg, peter.maydell, andrew, joel, qemu-arm, qemu-devel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] aspeed: Create SRAM name from first CPU index 2022-07-01 18:06 [PATCH] aspeed: Create SRAM name from first CPU index Peter Delevoryas @ 2022-07-02 6:01 ` Cédric Le Goater 2022-07-02 7:01 ` Peter Delevoryas 0 siblings, 1 reply; 5+ messages in thread From: Cédric Le Goater @ 2022-07-02 6:01 UTC (permalink / raw) To: Peter Delevoryas; +Cc: peter.maydell, andrew, joel, qemu-arm, qemu-devel 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] aspeed: Create SRAM name from first CPU index 2022-07-02 6:01 ` Cédric Le Goater @ 2022-07-02 7:01 ` Peter Delevoryas 2022-07-02 7:36 ` Peter Delevoryas 0 siblings, 1 reply; 5+ messages in thread From: Peter Delevoryas @ 2022-07-02 7:01 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Delevoryas, peter.maydell, andrew, joel, qemu-arm, qemu-devel 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; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] aspeed: Create SRAM name from first CPU index 2022-07-02 7:01 ` Peter Delevoryas @ 2022-07-02 7:36 ` Peter Delevoryas 2022-07-02 15:29 ` Peter Delevoryas 0 siblings, 1 reply; 5+ messages in thread From: Peter Delevoryas @ 2022-07-02 7:36 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Delevoryas, peter.maydell, andrew, joel, qemu-arm, qemu-devel 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; > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] aspeed: Create SRAM name from first CPU index 2022-07-02 7:36 ` Peter Delevoryas @ 2022-07-02 15:29 ` Peter Delevoryas 0 siblings, 0 replies; 5+ messages in thread From: Peter Delevoryas @ 2022-07-02 15:29 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Delevoryas, peter.maydell, andrew, joel, qemu-arm, qemu-devel 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; > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-02 15:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-01 18:06 [PATCH] aspeed: Create SRAM name from first CPU index Peter Delevoryas 2022-07-02 6:01 ` Cédric Le Goater 2022-07-02 7:01 ` Peter Delevoryas 2022-07-02 7:36 ` Peter Delevoryas 2022-07-02 15:29 ` Peter Delevoryas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).