Draft CHERI PR - #14
Conversation
JeanRochCoulon
left a comment
JeanRochCoulon
left a comment
There was a problem hiding this comment.
Hello,
The gap between master and Cheri is huge.
I started to feedback comments to how reduce the gap.
Cheers
| logic [2:0] size; // transaction size: 000=Byte 001=2Byte; 010=4Byte; 011=8Byte; 111=Cache line (16/32Byte) | ||
| logic [CVA6Cfg.DCACHE_SET_ASSOC_WIDTH-1:0] way; // way to replace | ||
| logic [CVA6Cfg.PLEN-1:0] paddr; // physical address | ||
| logic [CVA6Cfg.XLEN-1:0] data; // word width of processor (no block stores at the moment) |
There was a problem hiding this comment.
Would not be possible to redefine XLEN when CHERI is present instead of defining a new param CLEN?
There was a problem hiding this comment.
Thanks so much for taking a look. We appreciate there's lots to look at, but any early feedback is much appreciated! XLEN vs CLEN is a tricky one: XLEN is used for several things, which are the same without CHERI, but different with CHERI. Most notably, all arithmetic (add, sub, xor, ...) in the ALU still happens on XLEN bits (64 in RV64) with or without CHERI. However, the values being forwarded through the pipeline grow to 128(+1) bits (CLEN) with CHERI. Something needs to happen to separate these. It might be that the RTL code is cleaner by introducing a separate parameter (something like ARITH_LEN) in the ALU and setting XLEN to 128, but that's a bit confusing for people who read the spec and see that XLEN is defined by the architecture is still supposed to be 64 for RV64Y (i.e. CHERI with 128 bit capabilities). We'll consider carefully what minimises the diff!
There was a problem hiding this comment.
Using several variables and maybe remove XLEN would not hurt me. To be elaborated.
| end | ||
|
|
||
| for (genvar i = 0; i < NR_READ_PORTS; i++) begin | ||
| for (genvar i = 0; i < NR_READ_PORTS; i++) begin : gen_read_register |
There was a problem hiding this comment.
PR to be done on master to reduce gap between master and CHERI branches
| // ALU branch compare result - ALU | ||
| input logic branch_comp_res_i, | ||
| // Brach unit result - ISSUE_STAGE | ||
| output logic [CVA6Cfg.VLEN-1:0] branch_result_o, |
There was a problem hiding this comment.
Better to redefine VLEN in relation to define a new param
| branch_exception_o.tval = '0; | ||
| branch_exception_o.tval2 = {CVA6Cfg.GPLEN{1'b0}}; | ||
| branch_exception_o.tinst = '0; | ||
| branch_exception_o.gva = CVA6Cfg.RVH ? v_i : 1'b0; |
There was a problem hiding this comment.
do not change this code, Verible will format it by its own
There was a problem hiding this comment.
Thanks: we run Verible on our internal PRs, and it must have reformatted this, presumably because the surrounding context changed and changed its heuristic for whether the lines should be aligned. I wonder if there's a way to avoid this creating spurious diffs. I guess if we don't run it ourselves and we raise the PR against master, the first thing that will happen is someone will apply all the Verible-suggested fixes and the diff will reappear anyway?
There was a problem hiding this comment.
The command reported into CONTRIBUTING.md file (the last line) need to be executed by the contributor before submitting the PR.
There was a problem hiding this comment.
Okay, as far as I can tell this diff is a result from doing that.
Remove XOR (which does nothing on zcheri), but more importantly, allow proper capability register decoding to bypass the shift select that is required for narrower loads.
Also, get rid of the "b" ones, since they were not used. This was due to being uncertain of the types when setting bits, and due to there potentially being a problem in FPGA with this path.
Use SetAddr for CADD to get rid of dedicated IncOffset logic. Also, get rid of special casing for immidiate variants, as it appears that operand_b already holds the immediate value due to someone between decode and execute fixing things up. (This follows ADDI implementation in CVA6).
Commit already sets the address. Without this change, the tag may have already been cleared by the representability check.
Otherwise the second of two consecutive branches can sneak through and see the old PCC.
…load Tested on Genesys2 with Sandisk Ultra 16GB (red/grey)
This fixes an issue found by Louis-Emile Ploix and the Oxford team.
As pointed out by Louis-Emile, without this the other unused fields are undefined
This causes an error with newer openocd versions
This type just compares against values in the general fu_op. The bit widths had gotten out of sync, so pin it to be the same length. Note that this field is much bigger than it needs to be!
As well as being tidier, this avoids lint warnings that concatenations with unsized vectors are not allowed
This is a bit silly because the alu2 will immediately throw away the capability bits.
These can only contain integers, so don't need to be widened to caps
Accelerators cannot currently write capability results. This was already the case, but make it more explicit and resolve a lint warning.
Fortunately, since it was in the most signficiant field, this was harmless
This was hardwired to 1'b0 for TestRIG, but TestRIG now sets debug_enable low which forces the debug_req low more tidily
We do not yet fully implement the RVY SDExt spec, but in any case all capability exceptions should be disabled with debug_mode
There was no interesting CHERI composition here. This just needed some tweaks to Sail config to enable the extensions
This is a bug reported by Maxwell Pettet of the Oxford University Verification lab. An unsafe address change was performed based on the assumption that if the PCC calculated from the commit stage is used as the PCC then the issue check must have succeeded and so the new PC is in-bounds, and so in representable bounds. However, this calculated PCC is also used to determine EPCC in exception cases, including where the issue PCC was out of bounds. This could allow machine/supervisor mode to change bounds of a capability by carefully setting it up as a faulting PCC with an out-of-bounds address and then reading the produced EPCC. Note that there may be opportunities to optimise this fix. As noted in the comment, the metadata being extracted is the same as is already extracted in the issue_stage.
lowRisc have produced several patches that seem signficant for improving the ariane-ethernet. Some of these do not seem to work with our setup but pull in those that do via vendored patches for now.
|
Closing this as out-of-date. See openhwgroup/cva6#3382 for the PR to upstream. |
10 participants
A PR of the zcheri branch against the upstream commit we are based on to get a sense of scope of the diffs. Do not merge: this will be refined over time and split into smaller, more manageable PRs.