Skip to content
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

PR for adding Questa support for CVA6 #2532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kasunb-accelr
Copy link

This PR contains the necessary updates to run Questasim (version: 2023.2) for CVA6.

Copy link
Contributor

github-actions bot commented Oct 7, 2024

❌ failed run, report available here.

@MikeOpenHWGroup
Copy link
Member

Hi @kasunb-accelr, thanks for this PR. Adding support for Questa would be a significant addition. Some of your suggested updates are significant, and we will need several people to review these. Before doing that, I would like to understand why this PR changes some of the submodules - why is this necessary?

Also, GitHub is reporting that This branch is out-of-date with the base branch. Please refactor your PR to the base branch.

@MikeOpenHWGroup MikeOpenHWGroup added the Type:Enhancement For feature requests and enhancements label Oct 7, 2024
@kasunb-accelr
Copy link
Author

Hi @MikeOpenHWGroup, There is a small change in the core-v-verif submodule only.

@ASintzoff
Copy link
Contributor

Could you propose a clean pull request without commits adding/removing debugging statements and white spaces?

@JeanRochCoulon
Copy link
Contributor

Great. Can you rebase your PR?

@kasunb-accelr
Copy link
Author

kasunb-accelr commented Oct 11, 2024

yes. I have rebased it. @JeanRochCoulon @ASintzoff
Thank you

@JeanRochCoulon
Copy link
Contributor

JeanRochCoulon commented Oct 11, 2024

Thanks @kasunb-accelr but the submodules do not seem updated: core-v-verif, hpdcache and ridcv-compliance.

As you can read on this page, the PR is "out-of-date"

@kasunb-accelr
Copy link
Author

Hi guys @ASintzoff @JeanRochCoulon, I have submitted again can you guys recheck it? Thank you

@JeanRochCoulon
Copy link
Contributor

The cva6 configuration called cv32a60x is deprecated. Please do not use it. Configurations called cv32a65x or imac_sv32 or imafc_sv32 are preferred.

verif/sim/cva6.py Outdated Show resolved Hide resolved
verif/sim/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

✔️ successful run, report available here.

@kasunb-accelr kasunb-accelr force-pushed the cva6_questa branch 2 times, most recently from 3cfc4d0 to c6f5782 Compare October 14, 2024 11:12
Copy link
Contributor

✔️ successful run, report available here.

Copy link
Contributor

✔️ successful run, report available here.

1 similar comment
Copy link
Contributor

✔️ successful run, report available here.

@ASintzoff
Copy link
Contributor

hello @AnouarZajni could you take a look at this PR as there are some modifications in the UVM testbench?

@JeanRochCoulon
Copy link
Contributor

@kasunb-accelr This new questa-uvm is very promising. To maintain it over time, a CI job should be defined to use it. What do you think about adding in .gitlab-ci.yml the target in smoke job. In that way, the questa-uvm will be included in Thales CI, and all regression would be triggered.

Copy link
Contributor

✔️ successful run, report available here.

@kasunb-accelr
Copy link
Author

@JeanRochCoulon, I appreciate your feedback. I added "questa-uvm" to the .gitlab-ci.yml. I do not know if it is correct or not because I am new to this CI.

Copy link
Contributor

❌ failed run, report available here.

assign rvfi_csr_``csr_name``_if[i].rvfi_csr_rdata = rvfi_if.rvfi_csr_o.``csr_name``.rdata; \
assign rvfi_csr_``csr_name``_if[i].rvfi_csr_wdata = rvfi_if.rvfi_csr_o.``csr_name``.wdata; \
for (genvar i = 0; i < RVFI_NRET; i++) begin : rvfi_csr_if_blk_``csr_name``\
uvma_rvfi_csr_if#(uvme_cva6_pkg::XLEN) rvfi_csr_``csr_name``_if (\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Seems that interface name/generate block name is missing suffix "i" to be able to set it individually in config dB

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AnouarZajni,
Thank you for your feedback. I think block names can be accessed using "i" automatically. Like following examples

for (genvar i = 0; i < RVFI_NRET; i++) begin  : rvfi_csr_if
      uvma_rvfi_csr_if#(uvme_cva6_pkg::XLEN)   rvfi_csr_if_inst (
        .clk            (clknrst_if.clk),
        .reset_n        (clknrst_if.reset_n),
        .rvfi_csr_rmask (rvfi_if.rvfi_o[i].mem_rmask),
        .rvfi_csr_wmask (rvfi_if.rvfi_o[i].mem_wmask),
        .rvfi_csr_rdata (rvfi_if.rvfi_o[i].mem_rdata),
        .rvfi_csr_wdata (rvfi_if.rvfi_o[i].mem_wdata)
      );
   end
for (int j = 3; j < 32; j++) begin
            uvm_config_db#(virtual uvma_rvfi_csr_if )::set(null,"*", $sformatf("csr_mhpmevent%0d_vif%0d", j, i),         rvfi_csr_if[i].rvfi_csr_if_inst);
end

Please let me know is it correct or not. or how to fix that issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact, but depending on the simulator the generate block will have different names. It's better to give an explicit name in generate block to be reused in uvm_config_db::set

@JeanRochCoulon
Copy link
Contributor

The CI is failed, you can access the fail logs under https://riscv-ci.pages.thales-invia.fr/dashboard/

Copy link
Contributor

github-actions bot commented Nov 5, 2024

❌ failed run, report available here.

@JeanRochCoulon
Copy link
Contributor

JeanRochCoulon commented Nov 5, 2024

@valentinThomazic This PR is pending from a while. Can you help @kasunb-accelr by providing him some information to debug test failure ?

@valentinThomazic
Copy link
Contributor

Hey @kasun-buddhi, do the smoke-tests with questa-uvm work locally on your branch?
We don't have Questasim 2023.2 on Thales servers which run the Gitlab CI (we have 2022.4) so I was wondering if that could be the cause of the issue.

@ASintzoff
Copy link
Contributor

as already said privately last week to @kasunb-accelr the issue is related the basename for /verilog_src/uvm-1.2/src/uvm_pkg.sv is not properly defined.

When invoking vlog, do not use absolute path for /verilog_src/uvm-1.2/src/uvm_pkg.sv.

@valentinThomazic
Copy link
Contributor

valentinThomazic commented Nov 5, 2024

The QUESTASIM_HOME env var is not properly defined in our questa_bashrc and there seems to be license issues on our side. I will let you know when I know more

@valentinThomazic
Copy link
Contributor

Hi @kasunb-accelr we will not be able to add this job in the CI since we do not have a valid uvm license for questa anymore.
Could you remove the CI job and the debug you added in your last commit ?
Your PR cannot be tested on our side but could be merged. Sorry for the inconvenience.

@kasunb-accelr
Copy link
Author

Hi @kasunb-accelr we will not be able to add this job in the CI since we do not have a valid uvm license for questa anymore. Could you remove the CI job and the debug you added in your last commit ? Your PR cannot be tested on our side but could be merged. Sorry for the inconvenience.

Hi @valentinThomazic, Thank you for your feedback I will remove the recent changes

Copy link
Contributor

github-actions bot commented Nov 7, 2024

✔️ successful run, report available here.

@JeanRochCoulon
Copy link
Contributor

Hello @kasunb-accelr
I am ready to merge, but as documented in CONTRIBUTING.md. I must rebase to be able to merge. That's why I wait for your rebase to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Enhancement For feature requests and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants