-
Notifications
You must be signed in to change notification settings - Fork 646
Fix dsim
support
#2286
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
Fix dsim
support
#2286
Conversation
…via ibex_mem_intf_agent_pkg::
…csr[], protect with `ifndef DSIM
Hey Akash, thanks for the PR getting dsim support going again! This is looking pretty good. Please mark as ready for review when you're happy with the state of things. If there is still some minor breakages, it would be better to get the bulk of things merged and then follow up with some smaller PRs, just to keep this changeset from getting too large. |
Thanks @hcallahan-lowrisc! I'm still working on cleaning this up a bit and making sure that all the tests are consistently passing. Will let you know when it's ready! I will need help testing on Xcelium/VCS, as I don't have immediate access to those tools. |
Sounds great. We primarily test using Xcelium and VCS, and the "Ibex Private CI" actions that run against PRs use Xcelium, so seeing those checks pass is the main criteria to be aware of. |
Perfect! |
Hey @hcallahan-lowrisc, this is ready for review. Most of the dsim tests pass, so I think this can be reviewed and merged as a starting point as I root cause the remaining failures. Most are probably the same as the outstanding regression failures with xcelium |
Great. Will review this now. For a point of reference going forward, you can see the latest set of Ibex regression results (ran with Xcelium) here : https://ibex.reports.lowrisc.org/opentitan/latest/report.html. (linked at the top of the Ibex README.md) |
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.
A couple of general comments:
- We use a rebase-only flow for this repo, so all the merge commits will need to be rebased away before merging.
- We also try not to have working commits adding/reverting changes within a PR, so those should all be dropped.
- Changes to vendored repos (in
vendor/
) shouldn't be applied directly, but rather as patches if we aren't going to push changes upstream. You can use thevendor.py
tool in OpenTitan to do this by 1) adding your changes as .patch files invendor/patches
, then 2) running<opentitan>/util/vendor.py vendor/google_riscv-dv.vendor.hjson --commit
to automatically create a commit that applies the new patch file. Let me know if you need any help doing this. - Generally we try and squash changes into a few concise commits before merging, rather than many smaller commits as you might do while doing development. For example, most of the changes to
rtl_simulation.yaml
could be squashed into maybe 1 or 2 commits. Overall, I think less than 8 commits for this many changes would be ideal, something like commits that fix the build system, fixup DV code incompatabilities, fixup RTL incompatabilities, apply vendoring fixes, fix existing bugs and formatting errors.
But other than the minor style things above, this looks great! I will try and get a dsim installation going and run through a regression myself, but I think this should be easy to approve and merge. Thanks again!
# dsim does not link image.so with ISS_LDFLAGS and ISS_LIBS, so symbols are missing | ||
# so we need to re-link image.so with the linker flags and libraries | ||
- gcc -shared -Bsymbolic -o <tb_dir>/image.so @<tb_dir>/obj/objfiles <ISS_LDFLAGS> <ISS_LIBS> -lstdc++ |
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.
Ah that's unfortunate it needs to be done as a seperate step. But this seems pretty clean. Thanks
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 definitely unfortunate. I tried everything to get dsim
to link it, but it doesn't seem possible
Could we potentially just squash this whole thing to one commit? |
And thanks for the feedback on the vendor patches, I will go do that! |
Also I have a nice Dockerfile setup for dsim+ibex if you'd like to speed up the process of testing out the dsim changes. Happy to share that if helpful |
I think a handful of commits is probably right for changes of this scope. Certainly we would want to keep the RTL changes on their own, and also any standalone bugfixes / cleanups to existing code should be isolated commits.
Thanks, that might be really useful if you could drop it in a comment. At lowRISC we've generally been leaning towards using Nix for dependency management and tool isolation, so I would prefer not to merge a Dockerfile at this point. But it will be a useful reference, thanks. |
No worries I can split it up; just thought I'd ask to save myself some work ;) |
Continuing this in #2297, which is ready for review. |
This PR fixes up Ibex UVM support for
dsim
simulator. Several things had to be fixed:dsim
requires that enum values must use enum constantsZERO
enum constant instead of'0
and0
for instr enums: dv/uvm/core_ibex/riscv_dv_extension/ibex_directed_instr_lib.svCPUCTRLSTS
,SECURESEED
) toprivileged_reg_t
and replace enum constants intoimplemented_csr[]
: vendor/google_riscv-dv/src/riscv_instr_pkg.sv, vendor/google_riscv-dv/euvm/riscv/gen/riscv_instr_pkg.d, dv/uvm/core_ibex/riscv_dv_extension/riscv_core_setting.tpl.svdsim
does not appear to respect "re-exports"ibex_mem_intf_agent_pkg::ADDR_WIDTH
andibex_mem_intf_agent_pkg::DATA_WIDTH
withibex_mem_intf_pkg::ADDR_WIDTH
andibex_mem_intf_pkg::DATA_WIDTH
: dv/uvm/core_ibex/tests/core_ibex_base_test.sv, dv/uvm/core_ibex/tests/core_ibex_test_lib.sv, dv/uvm/core_ibex/tests/core_ibex_vseq.svdsim
in rtl/ibex_core.svdsim
in dv/uvm/core_ibex/yaml/rtl_simulation.yamldsim
case forreloc_word
in dv/uvm/core_ibex/scripts/run_instr_gen.pydsim_compile_opts
withdsim_opts
and apply correct defparams and +defines in util/ibex_config.pyWith these fixes, I was able to get most of the UVM tests running with
SIMULATOR=dsim ITERATIONS=1
:Future enhancements:
dsim
failuresDockerfile
fordsim
setupdsim
METADATA-DIR
make variable issue