Hi Cedric
> Subject: Re: [PATCH v5 03/29] hw/intc/aspeed: Introduce dynamic allocation
> for regs array
>
> On 3/6/25 11:38, Jamin Lin wrote:
> > Currently, the size of the "regs" array is 0x2000, which is too large.
> > To save code size and avoid mapping large unused gaps, will update it
> > to only map the useful set of registers. This update will support
> > multiple sub-regions with different sizes.
> >
> > To address the redundant size issue, replace the static "regs" array
> > with a dynamically allocated "regs" memory.
> >
> > Introduce a new "aspeed_intc_unrealize" function to free the allocated
> > "regs"
> > memory.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > include/hw/intc/aspeed_intc.h | 2 +-
> > hw/intc/aspeed_intc.c | 12 +++++++++++-
> > 2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index 03324f05ab..47ea0520b5 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -27,7 +27,7 @@ struct AspeedINTCState {
> > MemoryRegion iomem;
> > MemoryRegion iomem_container;
> >
> > - uint32_t regs[ASPEED_INTC_NR_REGS];
> > + uint32_t *regs;
> > OrIRQState orgates[ASPEED_INTC_NR_INTS];
> > qemu_irq output_pins[ASPEED_INTC_NR_INTS];
> >
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 465f41e4fd..feb2c52441 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -289,7 +289,7 @@ static void aspeed_intc_reset(DeviceState *dev)
> > {
> > AspeedINTCState *s = ASPEED_INTC(dev);
> >
> > - memset(s->regs, 0, sizeof(s->regs));
> > + memset(s->regs, 0, ASPEED_INTC_NR_REGS);
>
> this is not the same size. s->regs is larger than ASPEED_INTC_NR_REGS.
Will fix it.
>
> > memset(s->enable, 0, sizeof(s->enable));
> > memset(s->mask, 0, sizeof(s->mask));
> > memset(s->pending, 0, sizeof(s->pending)); @@ -307,6 +307,7 @@
> > static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> >
> > sysbus_init_mmio(sbd, &s->iomem_container);
> >
> > + s->regs = g_malloc0(ASPEED_INTC_NR_REGS);
>
> please use g_new(....)
Will do
Thanks for review and suggestion.
Jamin
>
>
> Thanks,
>
> C.
>
>
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops,
> s,
> > TYPE_ASPEED_INTC ".regs",
> > ASPEED_INTC_NR_REGS << 2);
> >
> > @@ -322,12 +323,21 @@ static void aspeed_intc_realize(DeviceState *dev,
> Error **errp)
> > }
> > }
> >
> > +static void aspeed_intc_unrealize(DeviceState *dev) {
> > + AspeedINTCState *s = ASPEED_INTC(dev);
> > +
> > + g_free(s->regs);
> > + s->regs = NULL;
> > +}
> > +
> > static void aspeed_intc_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> >
> > dc->desc = "ASPEED INTC Controller";
> > dc->realize = aspeed_intc_realize;
> > + dc->unrealize = aspeed_intc_unrealize;
> > device_class_set_legacy_reset(dc, aspeed_intc_reset);
> > dc->vmsd = NULL;
> > }
