Re: [PATCH v5 03/29] hw/intc/aspeed: Introduce dynamic allocation for regs array

["Cédric Le Goater" <clg@kaod.org>]

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_lin@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.

>       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(....)


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