Re: [PATCH v4 02/23] hw/intc/aspeed: Support setting different register sizes

[Cédric Le Goater]

On 3/4/25 11:03, Jamin Lin wrote:

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

yes.

I have a question: where should I free the dynamically allocated memory for 
regs?

Could you please give me any suggestion?

It would be in an unrealize() handler.


Thanks,

C.