[rtl] Flush Zcmp state machine on exception or interrupt and make the committing instructions atomic

[SamuelRiedel]

Correctly reset the Zcmp expanded instruction state machine when a trap (exception or interrupt) occurs.

Previously, if a trap interrupts the execution of expanded instructions, the state machine retains its progress. This stale state will cause the processor to incorrectly resume or misinterpret the next Zcmp instruction, leading to an incorrect execution. This bug was reported by "Xingzhi Zhang" (https://github.com/zxz2004626).

I tested this fix manually for now. We still need to implement dedicated regression tests in the future.

[SamuelRiedel]

@zxz2004626 Can you please verify that those changes fix the bugs also on your side?

[zxz2004626]

@zxz2004626 Can you please verify that those changes fix the bugs also on your side?

Sure, I will test it and report to you later.

[zxz2004626]

Hi @SamuelRiedel , I carefully verified these patches. Your patches fix most of the problems. The state machine is capable of successfully resetting itself when encountering interruptions or exceptions. Thank you for your quick response and serious attitude!

However, there still some subtle problems. The spec (cm.popret and cm.popretz) statement
"The final section of pseudocode executes atomically, and only executes if the section above completes without any exceptions or interrupt"
requires that adjusting the stack register, writing to the a0 register and returning should be executed atomically. The state machine transient sequence is CmIdle->CmPopLoadReg->CmPopIncrSp->CmPopZeroA0->CmPopRetRa. If the state machine is interrupted and reset after completing the CmPopIncrSp, the sp will be adjusted two times after the replay finished. I have also built a PoC for this and proved it. So CmPopIncrSp->CmPopZeroA0->CmPopRetRa should be executed atomically.

I have some improvements for this. You could see my PR in your fork.

[SamuelRiedel]

Thank you for the verification. And you're right, we'll fix that as well.

[zxz2004626]

Hi @SamuelRiedel,

I would like to know if the fix for this bug is still being worked on. Have there been any suggestions to my PR ? I am more than willing to assist in advancing this matter.

[SamuelRiedel]

Hi @zxz2004626

Sorry for the delay, this got a bit lost. We implemented a fix that also ensures the mv operations were not interruptable and that the debugging also works correctly. Finally, we also added a quick patch to ensure those instructions are correctly counted in the instruction retired counter. Feel free to try it out. I'll ask for feedback and hopefully we can merge this soon and close this issue.

[gautschimi]

looks good to me.

One comment is not really relevant to this PR but maybe you can comment on it.

[marnovandermaas]

This is a good change to go in. Should we open an issue to track that we eventually need to test this in the DV environment as well? Should we document how to do the manual testing in the meantime?

[SamuelRiedel]

Thank you both for the reviews. @marnovandermaas, yes definitely. I'll create an issue reporting what I did so far and with a proposal on how to implement something in riscv-dv.

[zxz2004626]

I think the PR is feasible. Thank you all for your contribution!