Draft: Add support for the CHERI "RV64Y" extension - #3382
Conversation
This reverts commit e7d7ea6. This is difficult to compose with CHERI, and there are currently no plans to enable both together. We will perform the sourcecode integration closer to ratification and upstreaming.
Some minor fixes to baseline CVA6 we need to get our test flows passing. We intend to merge these as individual PRs.
Without this, the CHERI changes seem to hit a compiler bug.
The newer Verilator version seems to trigger a different codepath in CI. This causes the Spike tandem to run, which fails without this change as Spike doesn't detect scalar crypto in the archstring. As a workaround for now, patch the subrepo to always enable sclar crypto: the configurations that test CVA6 with the extension removed don't test the instructions, so it doesn't matter that it's enabled in Spike.
The existing implementation adds commit logic to detect the first instruction after resume, then recalculates the architectural next PC in the CSR register file. Instead, we can detect the second instruction and use the usual mechanism to drop it. That way it has a calculated PC the CSR register file can use to determine the architectural state. This is useful for CHERI as there would be even more logic to recalculate the architectural next PC (and PCC).
This adds CHERI support to CVA6, according to version 0.9.4 of the riscv-cheri spec. Co-authored-by: joncapltd <Jonathan.Woodruff@capabilitieslimited.co.uk> Co-authored-by: Alexandre <alexandre@capabilitieslimited.co.uk> Co-authored-by: Bruno Sa <bruno.vilaca.sa@gmail.com>
|
Great @PRugg-Cap ! Great step ! |
cainria
left a comment
There was a problem hiding this comment.
Really partial review with a few questions for both the author and the project maintainers.
| localparam XLEN = cva6_config_pkg::CVA6ConfigXlen; | ||
| localparam CheriPresent = cva6_config_pkg::CVA6ConfigRVZcheripurecap; | ||
| localparam CLEN = CheriPresent ? 2 * XLEN : XLEN; | ||
| localparam REGLEN = CheriPresent ? $bits(cva6_cheri_pkg::cap_reg_t) : XLEN; | ||
| localparam logic [REGLEN-1:0] REG_NULL = CheriPresent ? cva6_cheri_pkg::REG_NULL_CAP : '0; | ||
| localparam logic [REGLEN-1:0] REG_ROOT = CheriPresent ? cva6_cheri_pkg::REG_ROOT_CAP : '0; |
There was a problem hiding this comment.
Have you considered using the config structure instead?
There was a problem hiding this comment.
It would be nice. The problem is that I want to introduce the functions reg_to_x and x_to_reg, which need to take a REGLEN and return an XLEN, and vice versa. That means they can't be taken from the config struct, as they have to be more static. CVA6Cfg.CLEN and CVA6Cfg.REGLEN are used elsewhere where they can be.
There was a problem hiding this comment.
If I'm misunderstanding something about how the types can be used, do let me know though!
There was a problem hiding this comment.
This is indeed a limitation of the language (it lacks generic types).
What I recommend in this case:
- If the function is called only in one module (not the case here), move the function into this module. This is what we did for mult functions.
- Otherwise, create a module instead of a function to allow a generic interface. But SystemVerilog does not allow to instantiate modules with call syntax so it might not be handy…
I would still be in favor of the component approach to not break the configuration effort.
There was a problem hiding this comment.
Hmm, okay, these functions were introduced to reduce diff and avoid lots of:
if (CheriPresent) begin
a_reg = {{REGLEN-XLEN}{1'b0}, a};
end else begin
a_reg = a;
end
If we need a module instantiation, then it's probably better to just go back to that.
There are currently 46 calls to these functions throughout the codebase.
I'll contemplate if there's a tidier way. Any chance you could elaborate on how the current approach breaks the configuration effort, so that I can understand the constraints?
There was a problem hiding this comment.
Or maybe
a_reg = CVA6Cfg.CheriEn ? {{REGLEN-XLEN}{1'b0}, a} : a;To put it short, we would like to be able to instantiate simultaneously 2 ariane cores with Cheri enabled and different XLEN.
Having values in the package instead of the config struct prevents us from doing that.
There was a problem hiding this comment.
Yeah, we had that form in some cases as well. The zero replicate does seem to generate warning in some cases though. Okay, thanks for the summary of the goal: I'll contemplate how to proceed.
| end | ||
| // 8-bytes size | ||
| 3'b011: begin | ||
| case (addr[3:0]) |
There was a problem hiding this comment.
Would the following shorter code be correct?
if (addr[3:0] <= 16) begin
return 16'h00ff << addr[3:0];
end
There was a problem hiding this comment.
I believe so. This mirrors the coding style used in the existing be_gen function just below. We assumed it was expressed this way for a reason, but I guess we should evaluate and possibly change both to be more concise.
| @@ -1,4 +1,6 @@ | |||
| // Copyright 2017-2019 ETH Zurich and University of Bologna. | |||
| // Copyright 2025 Bruno Sá and Zero-Day Labs. | |||
| // Copyright 2025 Capabilities Limited. | |||
There was a problem hiding this comment.
Is this copyright compatible with the CVA6 project?
There was a problem hiding this comment.
They're the same as the ETH and Bologna ones just above. All the changes are of course made under the indicated license.
| [submodule "core/cache_subsystem/hpdcache"] | ||
| path = core/cache_subsystem/hpdcache | ||
| url = https://github.com/openhwgroup/cv-hpdcache.git | ||
| url = https://github.com/Capabilities-Limited/cv-hpdcache.git |
There was a problem hiding this comment.
We will need to revert this before merge
There was a problem hiding this comment.
Okay: we have some non-trivial changes to the HPDCache to add capability tag support, but agreed, we should merge those there then this will become a bump of the existing submodule.
| [submodule "vendor/zero-day/axi_tagcontroller"] | ||
| path = vendor/zero-day/axi_tagcontroller | ||
| url = https://github.com/Capabilities-Limited/axi_cheri_tagcontroller.git |
There was a problem hiding this comment.
Are we willing to add a new submodule?
There was a problem hiding this comment.
It doesn't seem like this is a question for us, but just to give context: the tag controller is needed to provide tagged memory, splitting tags out from the AXI requests coming out of the caches and storing them in a dedicated region of DRAM. It definitely "feels" like its own component, much like the HPDCache. If the question is whether we should vendor instead, then we'd likely be happy with that approach.
Auto whitespace fix Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Auto whitespace fix Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Verible is now failing because of the IS_XLEN{32,64} checking script. I did cherry-pick in the commit to fix it, but I'll hold off on pushing it for now because that would pollute the diff. We'll get it when we rebase anyway. |
| localparam CVA6ConfigRVZcheripurecap = 0; | ||
| localparam CVA6ConfigRVZcherihybrid = 0; |
There was a problem hiding this comment.
| localparam CVA6ConfigRVZcheripurecap = 0; | |
| localparam CVA6ConfigRVZcherihybrid = 0; |
There was a problem hiding this comment.
Is this suggestion because we shouldn't modify deprecated packages, or because we shouldn't have these localparams? If the latter, I can have a go at removing them, but I think we need them statically in some cases.
There was a problem hiding this comment.
These config packages are not all deprecated, we need to maintain them. The localparams are not used, that is why it is better to remove them
|
|
||
| config_pkg::cva6_cfg_t cfg; | ||
|
|
||
| cfg.XLEN = CVA6Cfg.XLEN; |
There was a problem hiding this comment.
CLEN means Cheri len, also when Cheri is disable, coding style can be improved.
Maybe this would bring confusion, but defining cfg.XLEN as below would remove a lot of line modifications. @PRugg-Cap or @Gchauvon Do you have a better suggestion ?
| cfg.XLEN = CVA6Cfg.XLEN; | |
| cfg.XLEN = CheriPresent ? (CVA6Cfg.XLEN * 2) : CVA6Cfg.XLEN; |
There was a problem hiding this comment.
Hmm, not quite. CLEN is "capability len" and stands for the architectural length of a capability (excluding tag). However, it's actually quite rarely used. We still need XLEN, which is the size of the x registers, e.g. throughout the frontend, and in the ALU. This matches what is written in the RISC-V architecture document. The parameter we use more broadly is REGLEN, which is bigger than CLEN because we store some extra microarchitectural partially decompressed metadata in the registers.
There was a problem hiding this comment.
I did a quick grep of the repository, filtering out submodules etc. After our changes the counts of uses of the different parameters are as follows:
CLEN: 198
REGLEN: 123
XLEN: 1038
If we changed XLEN to be CLEN, then we'd save those 198, but the 1038 XLENs would all have to be "ADDRLEN" or "ARITHLEN" or something, so it would actually make the diff a lot bigger.
There was a problem hiding this comment.
(Though I was surprised we use CLEN more than REGLEN... The parts of the code I was mostly interacting with were more REGLEN heavy)
There was a problem hiding this comment.
Though I do take the point that having "CLEN", e.g. on the path from memory into the pipeline, is confusing when CHERI is disabled out. We can consider another option, e.g. something like "MEMLEN" (better name needed!) which is the maximum width of something loaded in from memory in one cycle.
There was a problem hiding this comment.
Thanks for the explanations. I really prefer MEMLEN. I would be please to get @Gchauvon feedback to grant this modification (Guillaume worked on type strategy among the cva6)
| @@ -0,0 +1,183 @@ | |||
| // Copyright 2021 Thales DIS design services SAS | |||
There was a problem hiding this comment.
Using cv32a60x config package as example is better, it does not define useless localparams
There was a problem hiding this comment.
Okay: I'll give that a go
|
👋 Hi there! This pull request seems inactive. Need more help or have updates? Feel free to let us know. If there are no updates within the next few days, we'll go ahead and close this PR. 😊 |
|
Apologies: I'm still hoping to find time to do the type refactor, but hopefully we can leave this open. |
As discussed, at Capabilities Limited based on initial work at Zero Day Labs, we have been integrating CHERI (the RISC-V Y extension): see https://github.com/riscv/riscv-cheri. This adds "capabilities" in hardware to provide memory safety and compartmentalisation that allows software to protect itself against common security vulnerabilities like buffer overflows.
This is a draft PR with the current state of our changes to get feedback on a path to upstreaming. I have extracted out only the CHERI features from our fork https://github.com/Capabilities-Limited/cheri-cva6, along with the minimal changes required for tests to pass etc. Only the last commit "Add RV64Y support" is relevant for review: other things will be addressed in other PRs. It would be very helpful to get feedback on the code style, if there are any changes that can be extracted out and made upstream separately, and if there are any changes we can make to the CHERI implementation to reduce the diff.
This PR does not introduce CHERI-specific tests. We primarily use TestRIG for this testing. We understand this is a requirement for upstreaming, but the aim of this PR is to establish that we can integrate CHERI without regressing the baseline design. Since RVY is backwards compatible, all the existing tests in this repository we have tried pass, both with and without CHERI enabled in.
A few other notes on limitations: