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.