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