Zknd extension `aes64ks1i` decode error.

ha0lyu commented 2 weeks ago

Before start

[X] I have read the RISC-V ISA Manual and this is not a RISC-V ISA question. 我已经阅读过 RISC-V 指令集手册，这不是一个指令集本身的问题。
[X] I have read the XiangShan Documents. 我已经阅读过香山文档。
[X] I have searched the previous issues and did not find anything relevant. 我已经搜索过之前的 issue，并没有找到相关的。
[X] I have reviewed the commit messages from the relevant commit history. 我已经浏览过相关的提交历史和提交信息。

Describe the bug

Here is the picture shows that instr 0x310d1413 is decoded aes64ks1i s0,s10,784 by Xiangshan & NEMU, spike decoded it as aes64ks1i s0, s10, 0. Spike is right.

However, the commit instr trace shows 0x310d1413 is decoded correctly, but: Xiangshan & NEMU decode 0x317e9f13 as aes64ks1i t5, t4, 7, this is wrong. Spike still right: core 0: 0x000000008000017c (0x31ff9493) aes64ks1i s1, t6, 15. Spike is right because of a piece of asm code:

pseg_0:
    aes64ks1i x8, x26, 0
    aes64ks1i x9, x31, 15
    aes64ks1i x25, x19, 4

I am not sure the inside of Xiangshan & NEMU whether decode correctly, if they are right, I got this:

Expected behavior

Instruction trace show correct instructions. s1 is right.

To Reproduce

test.zip

Environment

XiangShan branch: master
XiangShan commit id: e80f666e9
NEMU commit id: fce68dcb
SPIKE commit id: 1.1.1-dev

Additional context

No response

NewPaulWalker commented 2 weeks ago

Here is the picture shows that instr 0x310d1413 is decoded aes64ks1i s0,s10,784 by Xiangshan & NEMU, spike decoded it as aes64ks1i s0, s10, 0. Spike is right.

Thank you for your issue. This is indeed a bug in NEMU. When decoding the aes64ks1i instruction, NEMU incorrectly selects the high 12 bits of the instruction as the immediate value, rather than the 4 bits specified in the manual. For example, for the instruction 0x310d1413, the decoded immediate value is 0x310, which corresponds to 784 in NEMU.

Additionally, NEMU does not check the immediate value range for aes64ks1i:
Note that rnum must be in the range 0x0..0xA. The values 0xB..0xF are reserved.

I will fix this bug and then check if a similar issue exists in XiangShan.

NewPaulWalker commented 2 weeks ago

I will fix this bug and then check if a similar issue exists in XiangShan.

I believe that this PR can fix this bug in NEMU. However, additionally, based on my tests, it seems that XiangShan does not check the value of rnum, and it does not report an exception for the instruction aes64ks1i s1, t6, 15.

ha0lyu commented 2 weeks ago

Thanks for your confirmation and PR. Could you tell me how do you run emu to check whether xiangshan check the value of rnum or other registers?

NewPaulWalker commented 2 weeks ago

Could you tell me how do you run emu to check whether xiangshan check the value of rnum or other registers?

I performed differential testing again with the fixed versions of NEMU and XiangShan. For the aes64ks1i s1, t6, 15 instruction, I found that XiangShan did not raise an illegal instruction exception.

Additionally, this PR should have fixed the issue.

ha0lyu commented 2 weeks ago

Thanks for your reply and PR.

NewPaulWalker commented 2 weeks ago

Thanks for your reply and PR.

It seems that there are indeed some issues with our implementation of the Cryptography extension. We welcome and look forward to you finding other bugs in the Cryptography extension.

ha0lyu commented 1 week ago

I will test it later after this bug fixed.

Thank you.

NewPaulWalker commented 1 week ago

I will test it later after this bug fixed.

I will also double-check before merging this PR to ensure that no checks related to the Cryptography extension have been missed.

OpenXiangShan / XiangShan