Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dv, rv_core_ibex] Fix broken rv_core_ibex_nmi_irq_test #25929

Closed
wants to merge 1 commit into from

Conversation

ZhongYic00
Copy link

@ZhongYic00 ZhongYic00 commented Jan 17, 2025

dif_rv_core_ibex_enable_nmi does a write rather than set, so to enable both wdog and alert NMI, kDifRvCoreIbexNmiSourceAll(=wdog|alert) should be used.

With current called twice impl, as shown below, nmi_enable.wdog_en will be cleared by second call enabling NMISourceAlert.
image

@ZhongYic00 ZhongYic00 requested a review from a team as a code owner January 17, 2025 15:08
@ZhongYic00 ZhongYic00 requested review from alees24 and removed request for a team January 17, 2025 15:08
dif_rv_core_ibex_enable_nmi does a write rather than set, so to enable
both wdog and alert NMI, kDifRvCoreIbexNmiSourceAll(=wdog|alert) should
be used.
Signed-off-by: zhongyic <[email protected]>
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very sensible to me. I'll wait a little longer to give someone who knows more about Ibex SW than I do (just in case I'm missing something).

But I want to properly say thank you for the excellent PR, complete with a screenshot that shows the problem with the previous code.

@rswarbrick rswarbrick requested a review from jwnrt January 20, 2025 10:29
@jwnrt
Copy link
Contributor

jwnrt commented Jan 20, 2025

NMI_ENABLE is rw1s so you shouldn't be able to clear these bits by writing 0, right?

https://opentitan.org/book/hw/ip/rv_core_ibex/doc/registers.html?highlight=nmi_enable#nmi_enable

Are we looking at the wrong signals in this waveform, or is reggen not generating the correct RTL?

Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to block this for now. The actual software change is fine but should be semantically equivalent, so I want to investigate why this fixes the bug.

@ZhongYic00 did your waves come from VCS, Xcelium, Verilator..? If you happen to have the logs lying around then there should be two seeds printed in there somewhere (build seed and run seed) which would help us reproduce. Thanks!

This test is passing on FPGA so I'm slightly surprised it fails in sim.

@ZhongYic00
Copy link
Author

@jwnrt I'm sorry for mistakenly thinking this was a bug in the test code due to my incomplete verification, and for misunderstanding the meaning of the silicon_params(tags = ["broken"]). At the same time, I appreciate your thoughtful implementation of the design verification code, which reflects the rw1s feature of NMI_ENABLE very well.

I'm new to the OpenTitan project and am participating in a bug hunting event that uses an previous version & modified RTL code. Upon my further verification, this issue was caused by a bug in the RTL.

Thank you both for your prompt replies! 🥰

@ZhongYic00 ZhongYic00 closed this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants