Conversation
33d63f0 to
f66937e
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Solidity uarch interpreter/step implementation to align with a newer machine-emulator layout that introduces a shadow TLB region and checkpointing support, plus related constant and ECALL interface updates.
Changes:
- Add TLB/checkpoint-related constants and extend uarch ECALL handling (halt/putchar/mark-dirty-page/write-TLB).
- Introduce
write4Wordsto efficiently write a full 32-byte leaf (used for TLB entry updates). - Update mock/test memory layout and CMIO RX buffer constant names to match the new address map.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/UArchInterpret.t.sol | Updates tests for new halt flag API and mock memory layout sizing. |
| templates/EmulatorConstants.sol.template | Adds TLB sizing + checkpoint address constants to template. |
| templates/AccessLogs.sol.template | Reorders prod/test sections and adds write4Words; updates mock memory layout to include shadow TLB. |
| src/UArchStep.sol | Adds new ECALL functions and uses new halt/cycle semantics. |
| src/SendCmioResponse.sol | Renames constants to updated address-map identifiers. |
| src/EmulatorConstants.sol | Regenerates constants and adds TLB/checkpoint constants. |
| src/EmulatorCompat.sol | Adds checkpoint hash helpers, changes halt flag API, and implements TLB ECALL write path. |
| src/AdvanceStatus.sol | New helper to interpret advance/yield status from HTIF tohost. |
| src/AccessLogs.sol | Adds write4Words and minor parameter naming cleanup in offset helpers. |
| shasum-download | Updates download checksums to match new emulator artifacts. |
| helper_scripts/generate_UArchStep.sh | Adjusts generator filtering/replacements for updated upstream C++ annotations/signatures. |
| helper_scripts/generate_SendCmioResponse.sh | Updates generator replacement for new RX buffer constants and hash-tree word size naming. |
| helper_scripts/generate_EmulatorConstants.lua | Extends constants generation for new address map / yield reasons / TLB config. |
| Makefile | Bumps referenced machine-emulator version for test data/log downloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f66937e to
6faa5ab
Compare
6faa5ab to
c9b255c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| uint64 tohost = | ||
| EmulatorCompat.readWord(a, EmulatorConstants.HTIF_TOHOST_ADDRESS); | ||
| uint16 reason = uint16(tohost >> 32); |
There was a problem hiding this comment.
This hardcoded 32 should ideally use a constant to make it more clear.
|
@GCdePaula Can you take a look? You left some comments related with the changes at #81 and #83, and seems like they were all addressed already, is that correct? Any remaining concerns? @stephenctw You originally wrote the checkpoint code, does everything look ok? Have in mind we need to change the checkpoint address from @diegonehab Is the TLB changes related to the new ECALLs correct? @mpernambuco Can you address my comments and update to the new emulator tag? |
|
|
||
| error InvalidReason(uint16 reason); | ||
|
|
||
| function advanceStatus(AccessLogs.Context memory a) |
There was a problem hiding this comment.
Personally I don't like how this advanceStatus was introduced in the repository without being used or tests, however I assume Dave is testing it somehow, so we don't need to delay this PR.
| using Buffer for Buffer.Context; | ||
| using Memory for uint64; | ||
|
|
||
| function getCheckpointHash(AccessLogs.Context memory a) |
There was a problem hiding this comment.
Personally I don't like how both getCheckpointHash/setCheckpointHash were introduced in the repository without being used or tests, however I assume Dave is testing it somehow, so we don't need to delay this PR.
There was a problem hiding this comment.
I think these should be renamed to RevertRootHash to match the emulator.
There was a problem hiding this comment.
@stephenctw can we rename this to match the emulator naming convention? I think revert root hash is much more informative than checkpoint hash.
There was a problem hiding this comment.
This is being renamed by #88 which we will be merged after merging this PR first.
Indeed. There is a mismatch. |
|
No description provided.