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