Add mtopi CSR #347
Add mtopi CSR #347
Conversation
Add support for the mtopi CSR (0x7C0) from RISC-V Advanced Interrupt Architecture (AIA). Provides read-only access to highest-priority pending interrupt information with IID and IPID fields.
There was a problem hiding this comment.
Thanks for your contribution!
AI Assistance:
Parts of the explanation and understanding of the mtopi CSR were assisted using an AI tool to summarize its behavior in the context of the RISC-V Advanced Interrupt Architecture (AIA). All code and implementation decisions were written and verified manually. NO CODES WERE GENERATED WITH THE HELP OF AI.
Thank you for explicitly stating your use of AI tool assistance. AFAIK, we currently do not have a policy on AI contributions. IMO, we should ban AI contributions, even for "just documentation/explanation". Such contributions have deep issues with plagiarism, and we cannot possibly verify the provenance of of the training data.
"Manually" rewriting suggestions from AI is not the same as manually implementing the changes. You're already contradicting yourself by saying that AI only helped by providing explanations/documentation, and that you manually rewrote code suggested by AI.
@romancardenas what are your thoughts on adding a section on AI to our CODE_OF_CONDUCT.md, or creating a CONTRIBUTING.md? After your recent back and forth on @KushalMeghani1644's contributions, how do you feel about their usage of AI?
Replace manual assertions with test_ro_csr_field! macro that follows the same pattern as test_csr_field! but adapted for read-only CSRs.
|
Hey @rmsyn thanks for putting time to my PR! I feel there is maybe a little misunderstanding upon the "AI DISCLOSURE" part, I have clearly stated in the PR desc that: "Parts of the explanation and understanding of the mtopi CSR were assisted using an AI tool to summarize its behavior in the context of the RISC-V Advanced Interrupt Architecture (AIA). All code and implementation decisions were written and verified manually. NO CODES WERE GENERATED WITH THE HELP OF AI." See the "parts" word! I meant that only a few little queries and all were asked from AI to understand how Mtopi actually works since I haven't studied much on this CSR, and the codes WERE NOT generated by AI, I had indeed went through the official docs of https://tools.cloudbear.ru/docs/riscv-aia-1.0-20230630.pdf to understand mtopi well. And also this line by you, ""Manually" rewriting suggestions from AI is not the same as manually implementing the changes. You're already contradicting yourself by saying that AI only helped by providing explanations/documentation, and that you manually rewrote code suggested by AI." I hadn't not manually rewritter what AI gave me, I stated in the "AI DISCLOSURE" section in capital words that, "NO CODES WERE GENERATED WITH THE HELP OF AI." Edit: Also just to be transparent, my previous PRs that were merged had 0 use of AI in the codes! Those PRs were purely made by my brain + hands :D Thank you :) |
- Add field boundary testing for both iid and ipid fields - Improve comments to clarify macro usage and pattern following - Ensure complete test coverage matching original test_csr_field! scope
|
Hey @rmsynI've updated the mtopi tests to use test helper macros as requested. Created a |
The official link to the AIA specification (from the RISC-V Atlassian repo) is a Google Docs link RISC-V AIA], maybe the cloudbear.ru link you posted is a mirror of that document? Regarding your previous contributions, that's fine if you indeed contributed all those changes yourself. I was asking for @romancardenas to give feedback on allowing your AI contirbutions after the experience of reviewing your previous contribution. |
Hey! sorry if I was too harsh in my previous comments. I was just trying to be as transparent as I can! cause I always find it fun to contribute to OSS projects, especially bare metal codes like RISCV. Apologize for the inconveniences caused by me here. I hope that I have cleared out that everything is fine now :) And yes the cloudbear.ru link is a mirror, I for some reason wasn't able to find the official docs from RISCV, thanks for providing the links though. |
Extend test_csr_field! macro with new arm for read-only multi-bit fields. Update mtopi tests to use standard macro instead of custom implementation.
|
Hey! @rmsyn (sorry for pinging if you were busy) I've added a new arm to the existing test_csr_field! macro instead of creating a separate one. The macro now supports read-only CSR fields with the pattern [start:literal, end:literal] and mtopi tests now use the standard macro. Thanks for the clarification! |
There was a problem hiding this comment.
Hey! @rmsyn (sorry for pinging if you were busy) I've added a new arm to the existing test_csr_field! macro instead of creating a separate one. The macro now supports read-only CSR fields with the pattern [start:literal, end:literal] and mtopi tests now use the standard macro. Thanks for the clarification!
No worries, and thanks for making the changes. I have a couple other suggestions, otherwise LGTM outside the question of AI contributions.
These methods were simple aliases for ipid() and iid() respectively, which doesn't follow the crate's pattern of only adding convenience methods that provide semantic value or logical operations.
|
Hi @rmsyn I have removed the helper functiosn as requested! I hope the changes are fine :) if there are more improvements remaining, I'd be happy to fix them :D |
This PR adds support for the
mtopi(Machine Top Priority Interrupt) CSR, which is part of the RISC-V Advanced Interrupt Architecture (AIA) extension.Changes:
mtopi.rsregister implementation at address 0x7C0register.rsiid(Interrupt ID): bits 16-27ipid(Interrupt Priority ID): bits 0-7Features:
Testing:
AI Assistance:
Parts of the explanation and understanding of the mtopi CSR were assisted using an AI tool to summarize its behavior in the context of the RISC-V Advanced Interrupt Architecture (AIA). All code and implementation decisions were written and verified manually. NO CODES WERE GENERATED WITH THE HELP OF AI.