Hi Cedric,
> Subject: Re: [PATCH v4 02/23] hw/intc/aspeed: Support setting different
> register sizes
>
> On 3/3/25 10:54, Jamin Lin wrote:
> > Currently, the size of the regs array is 0x2000, which is too large.
> > So far, it only use GICINT128 - GICINT134, and the offsets from 0 to 0x1000
> are unused.
> > To save code size, introduce a new class attribute "reg_size" to set
> > the different register sizes for the INTC models in AST2700 and add a
> > regs sub-region in the memory container.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > include/hw/intc/aspeed_intc.h | 1 +
> > hw/intc/aspeed_intc.c | 8 +++++---
> > 2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index 03324f05ab..ecaeb15aea 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -42,6 +42,7 @@ struct AspeedINTCClass {
> > uint32_t num_lines;
> > uint32_t num_ints;
> > uint64_t mem_size;
> > + uint64_t reg_size;
> > };
> >
> > #endif /* ASPEED_INTC_H */
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 033b574c1e..316885a27a 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -117,10 +117,11 @@ static void aspeed_intc_set_irq(void *opaque, int
> irq, int level)
> > static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned
> int size)
> > {
> > AspeedINTCState *s = ASPEED_INTC(opaque);
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > uint32_t addr = offset >> 2;
>
> 'addr' is a confusing name. As it is used as a register index, I think 'reg'
> would
> be more appropriate.
Will fix it.
>
> > uint32_t value = 0;
> >
> > - if (addr >= ASPEED_INTC_NR_REGS) {
> > + if (offset >= aic->reg_size) {
>
> This is a useless test since the memory region 's->iomem' is initialized below
> with size 'aic->reg_size'.
>
Sorry, I lost to remove it.
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Out-of-bounds read at offset 0x%"
> HWADDR_PRIx "\n",
> > __func__, offset); @@ -143,7 +144,7 @@
> static
> > void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> > uint32_t change;
> > uint32_t irq;
> >
> > - if (addr >= ASPEED_INTC_NR_REGS) {
> > + if (offset >= aic->reg_size) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Out-of-bounds write at offset 0x%"
> HWADDR_PRIx "\n",
> > __func__, offset); @@ -308,7 +309,7 @@
> static
> > void aspeed_intc_realize(DeviceState *dev, Error **errp)
> > sysbus_init_mmio(sbd, &s->iomem_container);
> >
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops,
> s,
> > - TYPE_ASPEED_INTC ".regs",
> ASPEED_INTC_NR_REGS << 2);
> > + TYPE_ASPEED_INTC ".regs", aic->reg_size);>
> > memory_region_add_subregion(&s->iomem_container, 0x0,
> > &s->iomem);
> >
> > @@ -351,6 +352,7 @@ static void aspeed_2700_intc_class_init(ObjectClass
> *klass, void *data)
> > aic->num_lines = 32;
> > aic->num_ints = 9;
> > aic->mem_size = 0x4000;
> > + aic->reg_size = 0x2000;
>
> the model still uses ASPEED_INTC_NR_REGS in :
>
> struct AspeedINTCState {
> ...
> uint32_t regs[ASPEED_INTC_NR_REGS];
> ...
>
> which is redundant and error prone IMO.
>
Do you mean using "g_malloc" instead of the static regs array?
If I understand your question correctly, I will make the following changes.
1. uint32_t regs[ASPEED_INTC_NR_REGS]; --> uint32_t *regs;
2. aspeed_2700_intc_class_init() {
regs = g_malloc(aic->reg_size);
}
3. static void aspeed_intc_realize(DeviceState *dev, Error **errp) {
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops, s,
TYPE_ASPEED_INTC ".regs", aic->reg_size);
}
I have a question: where should I free the dynamically allocated memory for
regs?
Could you please give me any suggestion?
Thanks-Jamin
> Thanks,
>
> C.
>
>
> C.
