Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates multiple hypervisor/MMU correctness fixes (guest time trap causes, hgatp WARL masking, mtval2 semantics, and widened guest-physical address handling) and adds directed tests to validate the behavior without a full OS.
Changes:
- Fix CSR-trap classification for guest
time/timehaccesses (illegal vs virtual-instruction). - Correct two-stage translation handling for widened GPAs, including
hgatproot alignment/WARL and fault reporting. - Add directed hypervisor tests (C/asm) plus a Scala unit test for MMU spec widths.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/hypervisor/widened_gpa.S | Guest payload to load/store via widened GPA. |
| tests/hypervisor/widened_gpa.c | Builds VS+G stage tables to validate 41-bit GPA path. |
| tests/hypervisor/SYNTHESIS.md | Documents synthesis deltas for widened G-stage path. |
| tests/hypervisor/runtime.S | Minimal runtime/trap handler for freestanding tests. |
| tests/hypervisor/README.md | Usage docs for running directed hypervisor tests. |
| tests/hypervisor/mtval2.c | Directed test for mtval2 on guest-page vs access faults. |
| tests/hypervisor/Makefile | Builds/runs the directed hypervisor ELF tests. |
| tests/hypervisor/link.ld | Linker script for freestanding test images. |
| tests/hypervisor/hgatp_warl.S | Directed test for hgatp reserved PPN bits reading as zero. |
| tests/hypervisor/guest_time.S | Directed test for time/timeh trap cause correctness. |
| tests/hypervisor/Dockerfile | Containerized build/run environment for the tests. |
| tests/hypervisor/.gitignore | Ignores built .elf artifacts. |
| src/test/scala/vexiiriscv/memory/GuestPhysicalWidthTest.scala | Unit tests for new MMU spec width variants. |
| src/main/scala/vexiiriscv/Param.scala | Selects widened MMU specs when hypervisor is enabled. |
| src/main/scala/vexiiriscv/misc/TrapPlugin.scala | Writes/clears tval2 only for the intended fault classes. |
| src/main/scala/vexiiriscv/misc/PrivilegedPlugin.scala | Refines guest time gating + virtual-trap signaling. |
| src/main/scala/vexiiriscv/memory/TranslatedDBusAccessPlugin.scala | Prevents unintended truncation of translated addresses. |
| src/main/scala/vexiiriscv/memory/StaticTranslationPlugin.scala | Exposes request/translated widths for translation service. |
| src/main/scala/vexiiriscv/memory/ShadowMmuPlugin.scala | Masks hgatp reserved PPN bits; supports x4 root alignment. |
| src/main/scala/vexiiriscv/memory/Service.scala | Makes translation interfaces width-aware (request vs translated). |
| src/main/scala/vexiiriscv/memory/MmuPlugin.scala | Carries widened GPAs; reports over-width GPAs as guest faults. |
| src/main/scala/vexiiriscv/fetch/FetchL1Plugin.scala | Uses width-aware translated addresses directly. |
| src/main/scala/vexiiriscv/fetch/FetchCachelessPlugin.scala | Uses width-aware translated addresses directly. |
| src/main/scala/vexiiriscv/execute/lsu/LsuPlugin.scala | Uses width-aware translated addresses directly. |
| src/main/scala/vexiiriscv/execute/lsu/LsuCachelessPlugin.scala | Uses width-aware translated addresses directly. |
| src/main/scala/vexiiriscv/execute/CsrService.scala | Adds decode-time “virtual access” marking hook. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ARG REPOSITORY=https://github.com/flaviens/VexiiRiscv.git | ||
| ARG REF=fix/hypervisor-correctness |
|
Sure, on it |
d4aa950 to
d5921e7
Compare
|
I do not think merge them into only one make the things better to understand. If I understand correctly, You just generated the code from some AI agent, did you? If so, I have some serious questions for you: do you understand the change you made? do you review all the generated code and judge whether is is meaningless/have a good behavior? At least I see the test |
|
It is correct that I do not have a good understanding of the codebase. AI is heavily involved. If you want an expert on it, I'm more than happy to retract and let them do. |
It is important that one should take responsibility for his contributions, it is similar to a paper. And if you have a hard time on writing a fix/understanding this from AI. I think leaving an issue with fully reproducible is also a good contribution, and other will remember you as a good reporter ^^. To be honest, I also used codex a lot for developing (also in this project). But the most time I need to improve the thing a lot as even the highest model can produce many mistakes.... This is why I sent this question to you.
At least I think you change on the guest |
|
Hi @inochisa very happy to hear something positive from you! 😊 (first time I think?) Unfortunately really I cannot put the required effort here to have something I'd be happy about (indeed most of the time LLMs are good first shot for small fixes but this case is rabbitholing a bit and needs more guidance). Sorry for any inconvenience caused. |
Yeap, I apology for this strict attitude. I am kind of disappointed for non-reviewed content (I see too many similar low-quality things from my colleague and it is very hard for me to control myself XD.)
Yes, that's true. A simple fix is a good point. For guest timer CSR fix. I suggest you using the following method for a start:
The most important thing is that you should need to know what are you doing. This will make your PR convincing, and give you the courage to argue with others ^^ (Reviewer can also make mistakes, for me, I made a lot of mistakes.) Hope you can have a good start. |
|
I've been reporting the bugs, this I know how to do and happy to have argued with you 😗 the fix in a design that is unfamiliar to me is outside my territory ^^' |
|
To jump into the discussion, I think we should be careful with LLM, because they are very good a generating things that look good and plausible. It's a bit of a trap, because in open source the baseline is to suppose people are acting in good faith (and contributor using LLM are) but LLM do not have faith, they just aim at being plausible. And being plausible is all good and needed in movies and novels, but with real hardware stuffs, you must be right, not plausible. So it's important to label correctly any information in PR and issue. Like we used (and still do) to indicate "I'm not an expert in this field", "it works on my machine but not sure it's very portable", I think we should label any text or code generated by LLM as such, and detail how we checked it is correct. Lately someone pointed me to the following document about the subject and I find it worth the read: https://github.com/zizmorcore/.github/blob/main/AI_POLICY.md |
Consolidates #164, #165, #166 and #167.