-
Notifications
You must be signed in to change notification settings - Fork 877
Add verification support on cva6_reorg branch to #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add verification support on cva6_reorg branch to #685
Conversation
MikeOpenHWGroup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me @ASintzoff, thanks for this! I have made a couple of review comments, but these are for discussion, they do not need to gate the merge. Having said that, I will not approve the merge since this should come from @zarubaf.
.gitmodules
Outdated
| [submodule "corev_apu/riscv-dbg"] | ||
| path = corev_apu/riscv-dbg | ||
| url = https://github.com/pulp-platform/riscv-dbg.git | ||
| url = https://github.com/JeanRochCoulon/riscv-dbg.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fork riscv-dbg to an OpenHW Group repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not fork if possible.
IMO, it seems better to modify riscv-dbg according to pulp-platform/riscv-dbg#83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @JeanRochCoulon and I agree that the problem he was seeing because of a bug in the debug unit that should have been fixed by later commits. Can you try updating to the latest riscv-dbg module? We need to merge this before we can merge this PR as the public CVA6 can't depend on "private" forks of members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With latest version of riscv-dbg, the problem disappears. PR has been adapted accordingly.
core/include/riscv_pkg.sv
Outdated
| ModeSv64 = 11 | ||
| } vm_mode_t; | ||
|
|
||
| `ifdef CV32A6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can do this without using compiler macros (`defines)?
core/include/riscv_pkg.sv
Outdated
| ModeSv64 = 11 | ||
| } vm_mode_t; | ||
|
|
||
| `ifdef CV32A6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can do this without compiler macros (`defines)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. @JeanRochCoulon will address it. Details will follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XLEN is defined in a package. As there is no way to override a parameter in a package, we can create different versions of the same package and choose which version to compile. By the way, Flist.cva6 should be duplicated into Flist.cv32a6 and Flist.cv64a6. In that way, all the supported configurations (to be referred to cva6 requirement specification) would correspond to a dedicated FLIST file as FLIST.cva64a6_noFD for instance (naming to be fine-tuned of course).
@zarubaf @MikeOpenHWGroup, I can't wait for having your feedback !?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you could fork that out into two separate packages cv64a_config_pkg.sv and cv32a_config_pkg.sv and include the one that you are looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that, at a certain point, we are better off if we generate part of the RTL from templates as we are hitting on the limits of SV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you will see, a RTL proposal has been pushed. A new package has been created to host the cva6 configuration. Two Flist.cva6xxx files have been created, one to point on the package file where XLEN=64 and the other to the package file with XLEN=32. According to the variant, the Makefile selects the right Flist.cva6xxx file. Tell me if it works for you! Cheers
zarubaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks okay to me:
- We need to get the debug thing sorted.
- CI is failing
- I really don't like the unrolled interfaces, what is the rationale?
- Small nits
core/ariane.sv
Outdated
| // memory side, AXI Master | ||
| output ariane_axi::req_t axi_req_o, | ||
| input ariane_axi::resp_t axi_resp_i | ||
| output ariane_axi::id_t aw_id_o, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strongly prefer the struct variant. Why would you unroll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zarubaf As discussed, the structures generate issues at dc_shell synthesis time. To solve the problem, a wrapper has been added in core testbench (testbench used for UVM) following the suggestion, testharness is kept unchanged.
corev_apu/tb/ariane_testharness.sv
Outdated
| .clk_i(clk_i), | ||
| .rst_ni(rst_ni), | ||
| .rvfi_i(rvfi) | ||
| ) ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) ; | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Commit has been fixed.
core/include/riscv_pkg.sv
Outdated
| ModeSv64 = 11 | ||
| } vm_mode_t; | ||
|
|
||
| `ifdef CV32A6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you could fork that out into two separate packages cv64a_config_pkg.sv and cv32a_config_pkg.sv and include the one that you are looking for.
core/include/riscv_pkg.sv
Outdated
| ModeSv64 = 11 | ||
| } vm_mode_t; | ||
|
|
||
| `ifdef CV32A6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that, at a certain point, we are better off if we generate part of the RTL from templates as we are hitting on the limits of SV.
according to variant variable Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
New files: rvfi_pkg.sv, rvfi_tracer.sv, ariane_rvfi.pkg.sv - RVFI ports are added to ariane module - rvfi_tracer.sv is a module added in ariane-testharness.sv - RVFI_TRACE enables RVFI trace generation Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch> Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
0ed68ae to
36a3749
Compare
Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
when using 4 threads, tests from riscv-compliance and riscv-tests test suite are randomly stucked with rv32ima configuration Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
Thales synthesis workflow does not manage comments at end of lines Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
8608ea6 to
6a8d9c7
Compare
zarubaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
| @@ -0,0 +1,153 @@ | |||
| ////////////////////////////////////////////////////////////////////////////// | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpick: The isa string is imafdc not imacfd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, super nitpick!!!! :-) Fixed by commit
|
|
||
| package cva6_config_pkg; | ||
|
|
||
| localparam cva6_config_XLEN = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: According to the style guide localparams are supposed to be CamelCase.
| localparam cva6_config_XLEN = 32; | |
| localparam CVA6ConfigXlen = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by commit
|
|
||
| package cva6_config_pkg; | ||
|
|
||
| localparam cva6_config_XLEN = 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by commit
ea22990 to
aedf13f
Compare
|
@zarubaf Concerning the RFVI interface add-on, I am confused: I though @MikeOpenHWGroup would like to limit/remove all the ifdef from CVA6 ?! Mike, could you confirm it? |
core/include/riscv_pkg.sv
Outdated
| * Description: Common RISC-V definitions. | ||
| */ | ||
|
|
||
| import cva6_config_pkg::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move that into the package. It otherwise pollutes the namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
What we can't change is the top-level interface. (well we can but that would mean a major version bump - that is not necessary imho). Other approach would be to promote |
a48b0f4 to
656d14c
Compare
|
Ok, understood, thanks for the clarification, I have added a RVFI_TRACE ifdef in ariane module. |
|
@zarubaf @MikeOpenHWGroup Very good, we have solved the riscv-dbg issue, managed the different configurations (will be useful), and added RVFI. Splendid. |
| .clk_i ( clk ), | ||
| .rst_ni ( ndmreset_n ), | ||
| .boot_addr_i ( ariane_soc::ROMBase ), // start fetching from ROM | ||
| .hart_id_i ( '0 ), | ||
| .irq_i ( irq ), | ||
| .ipi_i ( ipi ), | ||
| .time_irq_i ( timer_irq ), | ||
| .debug_req_i ( debug_req_irq ), | ||
| .axi_req_o ( axi_ariane_req ), | ||
| .axi_resp_i ( axi_ariane_resp ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this should be reverted to the struct-based version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modification removed from commit Support FPGA generation
zarubaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the small FPGA fix and then let's merge!
- ariane_xilinx.sv: fix AXI bus expansion - .gitignore, Makefile, run.tcl: fix paths Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com>
According to selected configuration, Makefile calls cv32a6_imac_sv0_config_pkg.sv or cv64a6_imac_sv39_config_pkg.sv Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
needed for dc_shell Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
…arams Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com>
656d14c to
4f088f4
Compare
zarubaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work. LGTM - merging.
Initial attempt to split core from APU. Signed-off-by: MikeOpenHWGroup <mike@openhwgroup.org> Co-authored-by: Jean-Roch Coulon <jean-roch.coulon@invia.fr> Compile `corev_apu` (#667) * Makefile verilates corev_apu * Cleanup README * Fix URL to repo * Cleaned-up Makefile verilates corev_apu Signed-off-by: Mike Thompson <mike@openhwgroup.org> Add extended verification support (#685) * Makefile, riscv_pkg.sv: Select C64A6 or CV32A6 according to variant variable Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * add RVFI tracer and debug support New files: rvfi_pkg.sv, rvfi_tracer.sv, ariane_rvfi.pkg.sv - RVFI ports are added to ariane module - rvfi_tracer.sv is a module added in ariane-testharness.sv - RVFI_TRACE enables RVFI trace generation Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch> Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Move example_tb from cva6 to core-v-verif project Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Makefile: remove useless rule for vsim Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: add timescale definition when vsim is used Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: add vcs support (fix #570) Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * rvfi_tracer.sv: fix compilation error raised by vcs Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: use only 2 threads for verilator when using 4 threads, tests from riscv-compliance and riscv-tests test suite are randomly stucked with rv32ima configuration Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Flist.cva6: cleanup for synthesis workflow Thales synthesis workflow does not manage comments at end of lines Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Support FPGA generation - ariane_xilinx.sv: fix AXI bus expansion - .gitignore, Makefile, run.tcl: fix paths Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * riscv-dbg: update to 989389b0 (to support 32-bit CVA6 debug) Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Create cva6_config_pkg to setup 32- or 64-bit configuration According to selected configuration, Makefile calls cv32a6_imac_sv0_config_pkg.sv or cv64a6_imac_sv39_config_pkg.sv Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Flist, ariane_wrapper.sv: add wrapper to expand rvfi and axi structures needed for dc_shell Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * cv*a6_*_pkg.sv, riscv_pkg.sv: (Fix) Use the camel case for the localparams Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * riscv_pkg.sv: clean-up the cva6_config_pkg import Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Makefile, ariane.sv: RVFI_TRACE define conditions RVFI port in ariane Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> Co-authored-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch>
* Initial repository re-organization (#662) Initial attempt to split core from APU. Signed-off-by: MikeOpenHWGroup <mike@openhwgroup.org> Co-authored-by: Jean-Roch Coulon <jean-roch.coulon@invia.fr> Compile `corev_apu` (#667) * Makefile verilates corev_apu * Cleanup README * Fix URL to repo * Cleaned-up Makefile verilates corev_apu Signed-off-by: Mike Thompson <mike@openhwgroup.org> Add extended verification support (#685) * Makefile, riscv_pkg.sv: Select C64A6 or CV32A6 according to variant variable Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * add RVFI tracer and debug support New files: rvfi_pkg.sv, rvfi_tracer.sv, ariane_rvfi.pkg.sv - RVFI ports are added to ariane module - rvfi_tracer.sv is a module added in ariane-testharness.sv - RVFI_TRACE enables RVFI trace generation Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch> Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Move example_tb from cva6 to core-v-verif project Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Makefile: remove useless rule for vsim Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: add timescale definition when vsim is used Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: add vcs support (fix #570) Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * rvfi_tracer.sv: fix compilation error raised by vcs Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Makefile: use only 2 threads for verilator when using 4 threads, tests from riscv-compliance and riscv-tests test suite are randomly stucked with rv32ima configuration Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Flist.cva6: cleanup for synthesis workflow Thales synthesis workflow does not manage comments at end of lines Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Support FPGA generation - ariane_xilinx.sv: fix AXI bus expansion - .gitignore, Makefile, run.tcl: fix paths Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * riscv-dbg: update to 989389b0 (to support 32-bit CVA6 debug) Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * Create cva6_config_pkg to setup 32- or 64-bit configuration According to selected configuration, Makefile calls cv32a6_imac_sv0_config_pkg.sv or cv64a6_imac_sv39_config_pkg.sv Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Flist, ariane_wrapper.sv: add wrapper to expand rvfi and axi structures needed for dc_shell Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * cv*a6_*_pkg.sv, riscv_pkg.sv: (Fix) Use the camel case for the localparams Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * riscv_pkg.sv: clean-up the cva6_config_pkg import Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> * Makefile, ariane.sv: RVFI_TRACE define conditions RVFI port in ariane Signed-off-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> Co-authored-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch> * Add lfsr.sv to manifest Signed-off-by: Mike Thompson <mike@openhwgroup.org> * Directory re-organzation * fpga/xilinx/xlnx_axi_dwidth_converter_dm_*: move files (#726) into the new file organisation Signed-off-by: André Sintzoff <andre.sintzoff@thalesgroup.com> * move mmu_sv32 and mmu_sv39, move bootrom, update path (#729) Signed-off-by: sjthales <sebastien.jacq@thalesgroup.com> Co-authored-by: Mike Thompson <mike@openhwgroup.org> Co-authored-by: Jean-Roch Coulon <jean-roch.coulon@thalesgroup.com> Co-authored-by: Florian Zaruba <zarubaf@iis.ee.ethz.ch> Co-authored-by: sébastien jacq <57099003+sjthales@users.noreply.github.com>
Note this PR is strongly linked to the corresponding PR on core-v-verif repository.
Modifications done here are needed to have cva6 verification provided in core-v-verif working after the cva6 reorganization.