hw/intc/loongson_liointc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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
在 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;
> }
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;
>> }
> .
>
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;
>>> }
>> .
>>
>
>
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>
于 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
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.
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
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
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.
© 2016 - 2026 Red Hat, Inc.