Hi, Philippe, On Wed, Nov 4, 2020 at 12:17 PM chen huacai <zltjiang...@gmail.com> wrote: > > Hi, Philippe and Jiaxun, > > On Wed, Nov 4, 2020 at 1:17 AM Philippe Mathieu-Daudé <f4...@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é" > > > <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[]. 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
