Hi Cedric,
> Subject: Re: [PATCH v5 04/29] hw/intc/aspeed: Support setting different
> register size
>
> On 3/6/25 11:38, 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_lin@aspeedtech.com>
> > ---
> > include/hw/intc/aspeed_intc.h | 2 +-
> > hw/intc/aspeed_intc.c | 22 +++++-----------------
> > 2 files changed, 6 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index 47ea0520b5..17cd889e0d 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -16,7 +16,6 @@
> > #define TYPE_ASPEED_2700_INTC TYPE_ASPEED_INTC "-ast2700"
> > OBJECT_DECLARE_TYPE(AspeedINTCState, AspeedINTCClass,
> ASPEED_INTC)
> >
> > -#define ASPEED_INTC_NR_REGS (0x2000 >> 2)
> > #define ASPEED_INTC_NR_INTS 9
> >
> > struct AspeedINTCState {
> > @@ -42,6 +41,7 @@ struct AspeedINTCClass {
> > uint32_t num_lines;
> > uint32_t num_ints;
> > uint64_t mem_size;
> > + uint64_t reg_size;
>
> The '_size' prefix describing a number of registers is confusing.
> 'nr_regs' would be better.
>
Will do
Thanks for your suggestion and review.
Jamin
>
> Thanks,
>
> C.
>
>
>
> > };
> >
> > #endif /* ASPEED_INTC_H */
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > feb2c52441..1c3dc3fce0 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -120,13 +120,6 @@ static uint64_t aspeed_intc_read(void *opaque,
> hwaddr offset, unsigned int size)
> > uint32_t reg = offset >> 2;
> > uint32_t value = 0;
> >
> > - if (reg >= ASPEED_INTC_NR_REGS) {
> > - qemu_log_mask(LOG_GUEST_ERROR,
> > - "%s: Out-of-bounds read at offset 0x%"
> HWADDR_PRIx "\n",
> > - __func__, offset);
> > - return 0;
> > - }
> > -
> > value = s->regs[reg];
> > trace_aspeed_intc_read(offset, size, value);
> >
> > @@ -143,13 +136,6 @@ static void aspeed_intc_write(void *opaque,
> hwaddr offset, uint64_t data,
> > uint32_t change;
> > uint32_t irq;
> >
> > - if (reg >= ASPEED_INTC_NR_REGS) {
> > - qemu_log_mask(LOG_GUEST_ERROR,
> > - "%s: Out-of-bounds write at offset 0x%"
> HWADDR_PRIx "\n",
> > - __func__, offset);
> > - return;
> > - }
> > -
> > trace_aspeed_intc_write(offset, size, data);
> >
> > switch (reg) {
> > @@ -288,8 +274,9 @@ static void aspeed_intc_instance_init(Object *obj)
> > static void aspeed_intc_reset(DeviceState *dev)
> > {
> > AspeedINTCState *s = ASPEED_INTC(dev);
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> >
> > - memset(s->regs, 0, ASPEED_INTC_NR_REGS);
> > + memset(s->regs, 0, aic->reg_size);
> > memset(s->enable, 0, sizeof(s->enable));
> > memset(s->mask, 0, sizeof(s->mask));
> > memset(s->pending, 0, sizeof(s->pending)); @@ -307,9 +294,9 @@
> > static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> >
> > sysbus_init_mmio(sbd, &s->iomem_container);
> >
> > - s->regs = g_malloc0(ASPEED_INTC_NR_REGS);
> > + s->regs = g_malloc0(aic->reg_size);
> > 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
> <<
> > + 2);
> >
> > memory_region_add_subregion(&s->iomem_container, 0x0,
> > &s->iomem);
> >
> > @@ -361,6 +348,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 >> 2;
> > }
> >
> > static const TypeInfo aspeed_2700_intc_info = {