[PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[AlexChen]

According to the loongson spec
(http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
that the ISR size of per CORE is 8, so here we need to divide
(addr - R_PERCORE_ISR(0)) by 8, not 4.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Alex Chen <alex.chen@huawei.com>
---
 hw/intc/loongson_liointc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c
index 30fb375b72..fbbfb57ee9 100644
--- a/hw/intc/loongson_liointc.c
+++ b/hw/intc/loongson_liointc.c
@@ -130,7 +130,7 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size)

     if (addr >= R_PERCORE_ISR(0) &&
         addr < R_PERCORE_ISR(NUM_CORES)) {
-        int core = (addr - R_PERCORE_ISR(0)) / 4;
+        int core = (addr - R_PERCORE_ISR(0)) / 8;
         r = p->per_core_isr[core];
         goto out;
     }
@@ -173,7 +173,7 @@ liointc_write(void *opaque, hwaddr addr,

     if (addr >= R_PERCORE_ISR(0) &&
         addr < R_PERCORE_ISR(NUM_CORES)) {
-        int core = (addr - R_PERCORE_ISR(0)) / 4;
+        int core = (addr - R_PERCORE_ISR(0)) / 8;
         p->per_core_isr[core] = value;
         goto out;
     }
-- 
2.19.1

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Jiaxun Yang]

在 2020/11/3 17:32, AlexChen 写道:
> According to the loongson spec
> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
> that the ISR size of per CORE is 8, so here we need to divide
> (addr - R_PERCORE_ISR(0)) by 8, not 4.
Hi Alex

Thanks!

That was my fault.. Per Core ISA is rarely used by kernel..

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Reported-by: Euler Robot <euler.robot@huawei.com>
Btw:
How can you discover this by robot?
Huawei owns real artifical intelligence technology lol :-）


- Jiaxun
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> ---
>   hw/intc/loongson_liointc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c
> index 30fb375b72..fbbfb57ee9 100644
> --- a/hw/intc/loongson_liointc.c
> +++ b/hw/intc/loongson_liointc.c
> @@ -130,7 +130,7 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size)
>
>       if (addr >= R_PERCORE_ISR(0) &&
>           addr < R_PERCORE_ISR(NUM_CORES)) {
> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>           r = p->per_core_isr[core];
>           goto out;
>       }
> @@ -173,7 +173,7 @@ liointc_write(void *opaque, hwaddr addr,
>
>       if (addr >= R_PERCORE_ISR(0) &&
>           addr < R_PERCORE_ISR(NUM_CORES)) {
> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>           p->per_core_isr[core] = value;
>           goto out;
>       }

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[AlexChen]

On 2020/11/3 17:53, Jiaxun Yang wrote:
> 
> 
> 在 2020/11/3 17:32, AlexChen 写道:
>> According to the loongson spec
>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
>> that the ISR size of per CORE is 8, so here we need to divide
>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
> Hi Alex
> 
> Thanks!
> 
> That was my fault.. Per Core ISA is rarely used by kernel..
> 
> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
> Btw:
> How can you discover this by robot?
> Huawei owns real artifical intelligence technology lol :-）
> 
> 

Thanks for your review.
EulerRobot is a virtualization software quality automation project that
integrates some tools and test suites such as gcc/clang make test, qemu ut,
qtest, coccinelle scripts and avocado-vt.
The code checking tool found there was a potential array out of bounds at
'r = p->per_core_isr[core]', since 'core' may be 7 which is bigger than
'per_core_isr' array size 3.
So we found this bug.

Thanks,
Alex

> - Jiaxun
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> ---
>>   hw/intc/loongson_liointc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c
>> index 30fb375b72..fbbfb57ee9 100644
>> --- a/hw/intc/loongson_liointc.c
>> +++ b/hw/intc/loongson_liointc.c
>> @@ -130,7 +130,7 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size)
>>
>>       if (addr >= R_PERCORE_ISR(0) &&
>>           addr < R_PERCORE_ISR(NUM_CORES)) {
>> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
>> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>           r = p->per_core_isr[core];
>>           goto out;
>>       }
>> @@ -173,7 +173,7 @@ liointc_write(void *opaque, hwaddr addr,
>>
>>       if (addr >= R_PERCORE_ISR(0) &&
>>           addr < R_PERCORE_ISR(NUM_CORES)) {
>> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
>> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>           p->per_core_isr[core] = value;
>>           goto out;
>>       }
> .
> 

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Philippe Mathieu-Daudé]

On 11/3/20 3:05 PM, AlexChen wrote:
> On 2020/11/3 17:53, Jiaxun Yang wrote:
>>
>>
>> 閸︼拷 2020/11/3 17:32, AlexChen 閸愭瑩浜�:
>>> According to the loongson spec
>>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
>>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
>>> that the ISR size of per CORE is 8, so here we need to divide
>>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
>> Hi Alex
>>
>> Thanks!
>>
>> That was my fault.. Per Core ISA is rarely used by kernel..

No board in QEMU use this device. So we are safe =)

>>
>> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Btw:
>> How can you discover this by robot?
>> Huawei owns real artifical intelligence technology lol :-閿涳拷
>>
>>
> 
> Thanks for your review.
> EulerRobot is a virtualization software quality automation project that
> integrates some tools and test suites such as gcc/clang make test, qemu ut,
> qtest, coccinelle scripts and avocado-vt.
> The code checking tool found there was a potential array out of bounds at
> 'r = p->per_core_isr[core]', since 'core' may be 7 which is bigger than
> 'per_core_isr' array size 3.
> So we found this bug.
> 
> Thanks,
> Alex
> 
>> - Jiaxun
>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>> ---
>>>   hw/intc/loongson_liointc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c
>>> index 30fb375b72..fbbfb57ee9 100644
>>> --- a/hw/intc/loongson_liointc.c
>>> +++ b/hw/intc/loongson_liointc.c
>>> @@ -130,7 +130,7 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size)
>>>
>>>       if (addr >= R_PERCORE_ISR(0) &&
>>>           addr < R_PERCORE_ISR(NUM_CORES)) {
>>> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
>>> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>>           r = p->per_core_isr[core];
>>>           goto out;
>>>       }
>>> @@ -173,7 +173,7 @@ liointc_write(void *opaque, hwaddr addr,
>>>
>>>       if (addr >= R_PERCORE_ISR(0) &&
>>>           addr < R_PERCORE_ISR(NUM_CORES)) {
>>> -        int core = (addr - R_PERCORE_ISR(0)) / 4;
>>> +        int core = (addr - R_PERCORE_ISR(0)) / 8;
>>>           p->per_core_isr[core] = value;
>>>           goto out;
>>>       }
>> .
>>
> 
> 

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Philippe Mathieu-Daudé]

On 11/3/20 10:32 AM, AlexChen wrote:
> According to the loongson spec
> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
> that the ISR size of per CORE is 8, so here we need to divide
> (addr - R_PERCORE_ISR(0)) by 8, not 4.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> ---
>  hw/intc/loongson_liointc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

For a model added in 2020, its code style is a bit
disappointing (leading to that kind of hidden bugs).
I'm even surprised it passed the review process.

Thanks for the fix.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Jiaxun Yang]

于 2020年11月3日 GMT+08:00 下午8:28:27, "Philippe Mathieu-Daudé" <f4bug@amsat.org> 写到:
>On 11/3/20 10:32 AM, AlexChen wrote:
>> According to the loongson spec
>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
>> that the ISR size of per CORE is 8, so here we need to divide
>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
>> 
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> ---
>>  hw/intc/loongson_liointc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
>For a model added in 2020, its code style is a bit
>disappointing (leading to that kind of hidden bugs).
>I'm even surprised it passed the review process.
>
>Thanks for the fix.
>
>Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

It was my proof of concept code.

Any suggestions on enhancement?
I'll have some free time afterwards so probably can do something.

Thanks.

-Jiaxun

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Philippe Mathieu-Daudé]

On 11/3/20 4:40 PM, Jiaxun Yang wrote:
> 于 2020年11月3日 GMT+08:00 下午8:28:27, "Philippe Mathieu-Daudé" <f4bug@amsat.org> 写到:
>> On 11/3/20 10:32 AM, AlexChen wrote:
>>> According to the loongson spec
>>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
>>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
>>> that the ISR size of per CORE is 8, so here we need to divide
>>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>> ---
>>>  hw/intc/loongson_liointc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> For a model added in 2020, its code style is a bit
>> disappointing (leading to that kind of hidden bugs).
>> I'm even surprised it passed the review process.
>>
>> Thanks for the fix.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> It was my proof of concept code.

Don't worry Jiaxun, this comment is not to you, but to
the QEMU community as a whole. We should have asked
improvements during review, and explain what could be
improve, what is not the best style but acceptable,
and what is good.

> Any suggestions on enhancement?
> I'll have some free time afterwards so probably can do something.

It is a bit awkward to not comment on a patch (diff).
Comment I'd have made:

- Add definition for 0x8 magic value in R_PERCORE_ISR()
- Replace R_PERCORE_ISR() macro my function
- Replace dead D() code by trace events
- Use a simple 32-bit implementation (pic_ops.impl.min/max = 4)
  to let the generic memory code deal with the 8-bit accesses
  to mapper[].

If you have time, today what would be more useful is to have
tests for the Loongson-3 board.

You can see some examples with the Malta board in the repository:

$ git-grep malta tests/acceptance/

Regards,

Phil.

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[chen huacai]

Hi, Philippe and Jiaxun,

On Wed, Nov 4, 2020 at 1:17 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 11/3/20 4:40 PM, Jiaxun Yang wrote:
> > 于 2020年11月3日 GMT+08:00 下午8:28:27, "Philippe Mathieu-Daudé" <f4bug@amsat.org> 写到:
> >> On 11/3/20 10:32 AM, AlexChen wrote:
> >>> According to the loongson spec
> >>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
> >>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
> >>> that the ISR size of per CORE is 8, so here we need to divide
> >>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
> >>>
> >>> Reported-by: Euler Robot <euler.robot@huawei.com>
> >>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> >>> ---
> >>>  hw/intc/loongson_liointc.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> For a model added in 2020, its code style is a bit
> >> disappointing (leading to that kind of hidden bugs).
> >> I'm even surprised it passed the review process.
> >>
> >> Thanks for the fix.
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > It was my proof of concept code.
>
> Don't worry Jiaxun, this comment is not to you, but to
> the QEMU community as a whole. We should have asked
> improvements during review, and explain what could be
> improve, what is not the best style but acceptable,
> and what is good.
>
> > Any suggestions on enhancement?
> > I'll have some free time afterwards so probably can do something.
>
> It is a bit awkward to not comment on a patch (diff).
> Comment I'd have made:
>
> - Add definition for 0x8 magic value in R_PERCORE_ISR()
> - Replace R_PERCORE_ISR() macro my function
> - Replace dead D() code by trace events
> - Use a simple 32-bit implementation (pic_ops.impl.min/max = 4)
>   to let the generic memory code deal with the 8-bit accesses
>   to mapper[].
>
> If you have time, today what would be more useful is to have
> tests for the Loongson-3 board.
As you suggested, LOONGSON_LIOINTC will be used in loongson3_virt.c
and it is defined in a .c file now, so this is a chance for me to
rework liointc.

Huacai
>
> You can see some examples with the Malta board in the repository:
>
> $ git-grep malta tests/acceptance/
>
> Regards,
>
> Phil.
>


-- 
Huacai Chen

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[chen huacai]

Hi, Philippe,

On Wed, Nov 4, 2020 at 12:17 PM chen huacai <zltjiangshi@gmail.com> wrote:
>
> Hi, Philippe and Jiaxun,
>
> On Wed, Nov 4, 2020 at 1:17 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 11/3/20 4:40 PM, Jiaxun Yang wrote:
> > > 于 2020年11月3日 GMT+08:00 下午8:28:27, "Philippe Mathieu-Daudé" <f4bug@amsat.org> 写到:
> > >> On 11/3/20 10:32 AM, AlexChen wrote:
> > >>> According to the loongson spec
> > >>> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
> > >>> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
> > >>> that the ISR size of per CORE is 8, so here we need to divide
> > >>> (addr - R_PERCORE_ISR(0)) by 8, not 4.
> > >>>
> > >>> Reported-by: Euler Robot <euler.robot@huawei.com>
> > >>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> > >>> ---
> > >>>  hw/intc/loongson_liointc.c | 4 ++--
> > >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> For a model added in 2020, its code style is a bit
> > >> disappointing (leading to that kind of hidden bugs).
> > >> I'm even surprised it passed the review process.
> > >>
> > >> Thanks for the fix.
> > >>
> > >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >
> > > It was my proof of concept code.
> >
> > Don't worry Jiaxun, this comment is not to you, but to
> > the QEMU community as a whole. We should have asked
> > improvements during review, and explain what could be
> > improve, what is not the best style but acceptable,
> > and what is good.
> >
> > > Any suggestions on enhancement?
> > > I'll have some free time afterwards so probably can do something.
> >
> > It is a bit awkward to not comment on a patch (diff).
> > Comment I'd have made:
> >
> > - Add definition for 0x8 magic value in R_PERCORE_ISR()
> > - Replace R_PERCORE_ISR() macro my function
> > - Replace dead D() code by trace events
> > - Use a simple 32-bit implementation (pic_ops.impl.min/max = 4)
> >   to let the generic memory code deal with the 8-bit accesses
> >   to mapper[].
I have reworked, but I haven't take the last suggestion (Use a simple
32-bit implementation). Because the mapper[] are naturely accessed in
8-bit, if use 32-bit implementation, the data type of mapper[] should
also be changed, which looks very strange.

Huacai
> >
> > If you have time, today what would be more useful is to have
> > tests for the Loongson-3 board.
> As you suggested, LOONGSON_LIOINTC will be used in loongson3_virt.c
> and it is defined in a .c file now, so this is a chance for me to
> rework liointc.
>
> Huacai
> >
> > You can see some examples with the Malta board in the repository:
> >
> > $ git-grep malta tests/acceptance/
> >
> > Regards,
> >
> > Phil.
> >
>
>
> --
> Huacai Chen



-- 
Huacai Chen

Re: [PATCH] hw/intc: Fix incorrect calculation of core in liointc_read() and liointc_write()

[Philippe Mathieu-Daudé]

On 11/3/20 10:32 AM, AlexChen wrote:
> According to the loongson spec
> (http://www.loongson.cn/uploadfile/cpu/3B1500/Loongson_3B1500_cpu_user_1.pdf)
> and the macro definition(#define R_PERCORE_ISR(x) (0x40 + 0x8 * x)), we know
> that the ISR size of per CORE is 8, so here we need to divide
> (addr - R_PERCORE_ISR(0)) by 8, not 4.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> ---
>  hw/intc/loongson_liointc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to mips-next.