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

[Cédric Le Goater]

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.

      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'.

          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.

Thanks,

C.


C.