Conversation
marnovandermaas
left a comment
There was a problem hiding this comment.
Thank you so much for putting this together. Once the comments are addressed I think we will be able to sign this one off!
There was a problem hiding this comment.
Can we rename this file cva6-cheri-d1-clock-audit.md or similar? It would also be nice if this was rederable markdown.
There was a problem hiding this comment.
Okay: I will do that if we're happy we want to keep it!
There was a problem hiding this comment.
Just to check: Am I okay to Resolve these, or would you prefer to do it once you've checked the comment is addressed?
There was a problem hiding this comment.
You can resolve whichever ones you think I don't need to have a second look at.
| @@ -0,0 +1,101 @@ | |||
| # CVA6-CHERI | |||
|
|
|||
| CVA6-CHERI in Mocha is imported from the [Capabilities Limited repo][capltd-cva6-cheri]. | |||
There was a problem hiding this comment.
Probably good to mention that there are some extra patches on top in the lowRISC fork.
|
|
||
| CVA6-CHERI in Mocha is imported from the [Capabilities Limited repo][capltd-cva6-cheri]. | ||
|
|
||
| ## CVA6-CHERI design specification |
There was a problem hiding this comment.
This should probably go in a file called cva6.md in this folder: https://github.com/lowRISC/mocha/tree/main/doc/ref
We should also reference it in https://github.com/lowRISC/mocha/blob/main/doc/ref/arch.md
|
|
||
| [riscv-spec]: https://github.com/riscv/riscv-isa-manual | ||
| [capltd-cva6-cheri]: https://github.com/Capabilities-Limited/cheri-cva6 | ||
| [cva6-cheri-docs]: https://github.com/Capabilities-Limited/cheri-cva6/tree/zcheri/docs |
There was a problem hiding this comment.
We should prefer to point to the vendored in versions of these: https://github.com/lowRISC/mocha/tree/main/hw/vendor/cva6_cheri/docs
| F (floating point) | ||
| D (double-precision floating point) | ||
| C (compressed instructions) | ||
| A (atomics) | ||
| Y (CHERI: both purecap and hybrid) | ||
| Zkn (scalar cryptography) | ||
| B (bit manipulation) | ||
| Zicond (branchless conditional instructions) | ||
| Sdext (debug support) | ||
| Sdtrig (debug trigger support) | ||
| Zicbom (cache management operations) |
There was a problem hiding this comment.
Probably good to make this a bullet list.
|
|
||
| | Type | Item | Status | Note/Collaterals | | ||
| |---------------|----------------------------|--------|------------------| | ||
| | Documentation | SPEC_COMPLETED | Done | The above section gives information on the specification for CVA6-CHERI, including links to the parent RISC-V and RISC-V CHERI (Y) specifications. |
There was a problem hiding this comment.
Once we move the spec to the doc/ref document we should link to it here.
| |---------------|----------------------------|--------|------------------| | ||
| | Documentation | SPEC_COMPLETED | Done | The above section gives information on the specification for CVA6-CHERI, including links to the parent RISC-V and RISC-V CHERI (Y) specifications. | ||
| | Documentation | CSR_DEFINED | Done | As the CPU itself, CVA6-CHERI does not have a memory mapped interface for CSRs. The CSRs are defined by the RISC-V specifications listed above and accessed internally within the core. | ||
| | RTL | CLKRST_CONNECTED | TODO | This is still in progress. I've checked up to about 3 modules deep: see `clock_audit.txt`. We should discuss whether/how this should be checked in. |
There was a problem hiding this comment.
I think it's fine to have it in the doc/proj. As suggested above we should rename it and make sure it is markdown, but fine to keep this as a separate file to avoid clutter in this one.
| | RTL | CLKRST_CONNECTED | TODO | This is still in progress. I've checked up to about 3 modules deep: see `clock_audit.txt`. We should discuss whether/how this should be checked in. | ||
| | RTL | IP_TOP | Done | CVA6-CHERI's top-level `cva6` module is defined in [`cva6.sv`][cva6-top] | ||
| | RTL | IP_INSTANTIABLE | Done | CVA6-CHERI's top-level module is instantiated in [`top_chip_system.sv`][cva6-inst-loc]. | ||
| | RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ... |
There was a problem hiding this comment.
Main question here is whether the data cache is configured as you would expect the final version to be. If yes, then happy for this to be checked off.
There was a problem hiding this comment.
@marnovandermaas On the PHYSICAL_MACROS_DEFINED_80, it looks like we have doubled the ICache size (16KB -> 32KB) since the version imported in Mocha. This was due to the observed sensitivity of Linux boot times to the ICache size. This means we'll miss the 80% accuracy requirement. I guess there are three options:
- Bump the size up in Mocha. This should be a simple patch.
- Waive the requirement and document the expected byte usage, as with the tag controller.
- Plan to keep the 16K ICache in the version vendored into Mocha.
Let me know what you think is the best path forwards.
There was a problem hiding this comment.
I think we can waive the requirement. Please link the following issue where we track this: #745
| | RTL | IP_INSTANTIABLE | Done | CVA6-CHERI's top-level module is instantiated in [`top_chip_system.sv`][cva6-inst-loc]. | ||
| | RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ... | ||
| | RTL | FUNC_IMPLEMENTED | Done | CVA6-CHERI's mainline functionality is implemented. The baseline processor implements the RISC-V specification, and CHERI features are implemented, including manipulating capabilities, enforcing capability checks, and propagating tags and capability metadata throughout the pipeline. This is demonstrating by booting the pure capability CHERI Linux within Mocha, and pure capability CheriBSD in the `COREV_APU` SoC on FGPA. | ||
| | RTL | ASSERT_KNOWN_ADDED | TODO | ... |
There was a problem hiding this comment.
Just putting a comment here so that we can mark it as resolved when the assert knowns are added.
Co-authored-by: Marno van der Maas <34654485+marnovandermaas@users.noreply.github.com>
I haven't been able to test these. I also need to apply the patch to the vendored repo.
I wonder if this should be PR'd into the lowRISC fork instead...
marnovandermaas
left a comment
There was a problem hiding this comment.
Ok I've left another review, I think we're nearly there. If you can address all the open comments that I've raised and then mark them as resolved (unless you need my further input). Please also clean up the commit history a bit. It's fine to introduce the assertions in one commit and then the documentation in another but there's no need to see edits of the same file. If that is too much work it is also fine for you to squash it all into one commit.
| | RTL | FUNC_IMPLEMENTED | Done | CVA6-CHERI's mainline functionality is implemented. The baseline processor implements the RISC-V specification, and CHERI features are implemented, including manipulating capabilities, enforcing capability checks, and propagating tags and capability metadata throughout the pipeline. This is demonstrating by booting the pure capability CHERI Linux within Mocha, and pure capability CheriBSD in the `COREV_APU` SoC on FGPA. | ||
| | RTL | ASSERT_KNOWN_ADDED | TODO | ... | ||
| | Code Quality | LINT_SETUP | DONE | Verilator warning waivers are added to [`top_chip_system.vlt`][waivers]. | ||
| | Code Quality | LINT_SETUP | Done | Verilator warning waivers are added to [`top_chip_system.vlt`][waivers]. |
There was a problem hiding this comment.
Nit: can we squash this commit with the previous one?
There was a problem hiding this comment.
Yeah, I was definitely expecting all these commits to get squashed. Just thought the history would be helpful for you to check all your comments were addressed. I'll rework the history
There was a problem hiding this comment.
Nit: can we squash the rename of the file with the commit that introduces clockaudit.txt?
There was a problem hiding this comment.
Instead of adding this patch, I have fixed and commited these output asserts on our fork see this commit
You should be able to vendor in the latest CVA6 from there with the following command:
util/vendor.py --update --commit hw/vendor/cva6_cheri.vendor.hjson | |---------------|----------------------------|--------|------------------| | ||
| | Documentation | SPEC_COMPLETED | Done | [The CVA6-CHERI design spec][cva6-cheri-spec] describes the specification for CVA6-CHERI, including links to the parent RISC-V and RISC-V CHERI (Y) specifications. | ||
| | Documentation | CSR_DEFINED | Done | As the CPU itself, CVA6-CHERI does not have a memory mapped interface for CSRs. The CSRs are defined by the RISC-V specifications listed above and accessed internally within the core. | ||
| | RTL | CLKRST_CONNECTED | TODO | This is still in progress. See the [clock audit][clock-audit]. |
There was a problem hiding this comment.
This is now done right?
| | RTL | CLKRST_CONNECTED | TODO | This is still in progress. I've checked up to about 3 modules deep: see `clock_audit.txt`. We should discuss whether/how this should be checked in. | ||
| | RTL | IP_TOP | Done | CVA6-CHERI's top-level `cva6` module is defined in [`cva6.sv`][cva6-top] | ||
| | RTL | IP_INSTANTIABLE | Done | CVA6-CHERI's top-level module is instantiated in [`top_chip_system.sv`][cva6-inst-loc]. | ||
| | RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ... |
There was a problem hiding this comment.
I think we can waive the requirement. Please link the following issue where we track this: #745
No description provided.