On Mon, 11 Jan 2021, Jiaxun Yang wrote:
> On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote:
>> I think R_END should be 0x60, Jiaxun, what do you think?
>
> U r right.
> The manual is misleading.

The R_END constant is also used in loongson_liointc_init() for the length 
of the memory region so you might want to revise that. If this is a 32 bit 
register then you should decide what R_END means? Is it the end of the 
memory region in which case the reg starts at R_END - 4 or is it the 
address of the last reg in which case the memory region ends at R_END + 4. 
From the above I think it's the address of the last reg so you'll probably 
need to add 4 in loongson_liointc_init() when creating the memory region.

Regards,
BALATON Zoltan

> Thanks.
>
> - Jiaxun
>
>>
>> Huacai
>>
>> On Mon, Jan 11, 2021 at 5:51 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> Hi Peter, Huacai,
>>>>
>>>> On 1/10/21 8:49 PM, Peter Maydell wrote:
>>>>> On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>>>
>>>>>> From: Huacai Chen <chenhuacai@kernel.org>
>>>>>>
>>>>>> As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc:
>>>>>> 1, Move macro definitions to loongson_liointc.h;
>>>>>> 2, Remove magic values and use macros instead;
>>>>>> 3, Replace dead D() code by trace events.
>>>>>>
>>>>>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Signed-off-by: Huacai Chen <chenhuacai@kernel.org>
>>>>>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Message-Id: <20201221110538.3186646-2-chenhuacai@kernel.org>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>>  include/hw/intc/loongson_liointc.h | 22 ++++++++++++++++++
>>>>>>  hw/intc/loongson_liointc.c         | 36 +++++++++++++-----------------
>>>>>>  2 files changed, 38 insertions(+), 20 deletions(-)
>>>>>>  create mode 100644 include/hw/intc/loongson_liointc.h
>>>>>
>>>>> Hi; Coverity complains about a possible array overrun
>>>>> in this commit:
>>>>>
>>>>>
>>>>>> @@ -40,13 +39,10 @@
>>>>>>  #define R_IEN                   0x24
>>>>>>  #define R_IEN_SET               0x28
>>>>>>  #define R_IEN_CLR               0x2c
>>>>>> -#define R_PERCORE_ISR(x)        (0x40 + 0x8 * x)
>>>>>> +#define R_ISR_SIZE              0x8
>>>>>> +#define R_START                 0x40
>>>>>>  #define R_END                   0x64
>>>>>>
>>>>>> -#define TYPE_LOONGSON_LIOINTC "loongson.liointc"
>>>>>> -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC,
>>>>>> -                         TYPE_LOONGSON_LIOINTC)
>>>>>> -
>>>>>>  struct loongson_liointc {
>>>>>>      SysBusDevice parent_obj;
>>>>>>
>>>>>> @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size)
>>>>>>          goto out;
>>>>>>      }
>>>>>>
>>>>>> -    /* Rest is 4 byte */
>>>>>> +    /* Rest are 4 bytes */
>>>>>>      if (size != 4 || (addr % 4)) {
>>>>>>          goto out;
>>>>>>      }
>>>>>>
>>>
>>> Expanding macros in the following:
>>>
>>>>>> -    if (addr >= R_PERCORE_ISR(0) &&
>>>>>> -        addr < R_PERCORE_ISR(NUM_CORES)) {
>>>>>> -        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>>
>>> if (addr >= (0x40 + 0x8 * 0) && addr < (0x40 + 0x8 * 4))
>>> ->
>>> if (addr >= 0x40 && addr < 0x60)
>>> int core = (addr - 0x40) / 8;
>>>
>>>
>>>>>> +    if (addr >= R_START && addr < R_END) {
>>>>>> +        int core = (addr - R_START) / R_ISR_SIZE;
>>>
>>> if (addr >= 0x40 && addr < 0x64)
>>> int core = (addr - 0x40) / 0x8;
>>>
>>> R_END seems to be off by 4 in the above. Should it be 0x60?
>>>
>>> Regards,
>>> BALATON Zoltan
>>>
>>>>> R_END is 0x64 and R_START is 0x40, so if addr is 0x60
>>>>> then addr - R_START is 0x32 and so core here is 4.
>>>>> However p->per_core_isr[] only has 4 entries, so this will
>>>>> be off the end of the array.
>>>>>
>>>>> This is CID 1438965.
>>>>>
>>>>>>          r = p->per_core_isr[core];
>>>>>>          goto out;
>>>>>>      }
>>>>>
>>>>>> -    if (addr >= R_PERCORE_ISR(0) &&
>>>>>> -        addr < R_PERCORE_ISR(NUM_CORES)) {
>>>>>> -        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>>>>> +    if (addr >= R_START && addr < R_END) {
>>>>>> +        int core = (addr - R_START) / R_ISR_SIZE;
>>>>>>          p->per_core_isr[core] = value;
>>>>>>          goto out;
>>>>>>      }
>>>>>
>>>>> Same thing here, CID 1438967.
>>>>
>>>> Thanks Peter.
>>>>
>>>> Huacai, can you have a look please?
>>>>
>>>> Thanks,
>>>>
>>>> Phil.
>>>>
>>>>
>>
>
>