Re: [PATCH v5 04/29] hw/intc/aspeed: Support setting different register size

[Cédric Le Goater]

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


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 = {