On 11/3/20 4:40 PM, Jiaxun Yang wrote: > 于 2020年11月3日 GMT+08:00 下午8:28:27, "Philippe Mathieu-Daudé" <f4...@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.ro...@huawei.com> >>> Signed-off-by: Alex Chen <alex.c...@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é <f4...@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.