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

[Issue-1252] Added the support to run latest riscv-arch-tests in core-v-verif CVA6 infrastructure #1254

Merged
merged 14 commits into from
Jul 11, 2022

Conversation

fatimasaleem
Copy link
Contributor

@fatimasaleem fatimasaleem commented Apr 21, 2022

This PR solves the issue mentioned in 1252.
I have updated the cva6 environment to make it compatible to run the latest riscv-arch-tests with the riscof-dev infrastructure.

What was Changed

The following major changes were made while setting up the cva6 environment

  • Created a new testlist yaml file and a new install script for riscv-arch-test as @ASintzoff suggested
    - dv-riscv-arch-tests.sh sets all the environment variables and then calls all required install scripts
    - Created an install-riscv-isa-sim.sh script which is installing riscv-isa-sim as in riscof-dev the riscv-target has been removed, and for using the spike simulator as an architectural test model, we needed the required dependency files (e.g model_test.h)
    - Created a separate testlist_riscv-arch-tests-cv64a6_imafdc_sv39.yaml file for running the riscv-arch-tests on cva6 with proper path and env variable setup, major additions are as follows
    1. Added compile-time macros, XLEN, TEST_CASE_1 - as suggested by @neelgala
    2. Updated the path of the env according to the riscof-dev dir structure
    3. Updated the arch-test target to use the locally installed spike

  • Updated the cva6_spike_log_to_trace_csv.py so that spike logging is aligned with CVA6 verilated model logging

  • Replaced the cva6/sim/link.ld with the latest one

Environment Variables

export RISCV=/path/to/tootchain
export RISCV_GCC=$RISCV/bin/riscv64-unknown-elf-gcc
export RISCV_OBJCOPY=$RISCV/riscv64-unknown-elf/bin/objcopy
export SPIKE_ROOT=$RISCV

Steps to Run

After all these changes the CVA6 core-v-verilf environment is running all the current riscv-arch-tests on the riscof-dev branch.
Steps to run:

git clone [email protected]:10x-Engineers/core-v-verif.git
cd core-v-verif
git checkout issue-1252-fix
source cva6/regress/dv-riscv-arch-test.sh

@fatimasaleem fatimasaleem marked this pull request as ready for review April 21, 2022 20:56
Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

Please rename dv-riscv-arch-tests.sh in dv-riscv-arch-test.sh (as the test suite repository name).

cva6/regress/dv-riscv-arch-tests.sh Outdated Show resolved Hide resolved
cva6/regress/dv-riscv-arch-tests.sh Outdated Show resolved Hide resolved
cva6/regress/dv-riscv-arch-tests.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

Why are you changing the end address of trampoline in spike?

Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

As link.ld is used by all tests for cva6, I'm wondering if they are still buildable and executable.

Are you facing any trouble with the previous version?
What is the source of the version you propose?

cva6/sim/.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

Last year, we faced lot of issues with verilator versions greater than 4.110. This is the reason to use it. If a recent version is running riscv-tests, riscv-compliance, and riscv-arch-test test suite, I would be pleased to switch.

For the moment, I prefer to not modify install-cva6.sh and if required for riscv-arch-test, install a recent version of verilator and to use it, modify VERILATOR_ROOT variable outside the scripts.

Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

As said on previous commit, could you confirm that all the existing tests are runnable with this newest verilator?

@fatimasaleem
Copy link
Contributor Author

Why are you changing the end address of trampoline in spike?

As also mentioned in the issue,

While logging the results of the spike and verilated model of CVA6, there is a mismatch of one instruction, i.e. the spike starts logging from pc value of 0x80000000 and the verilated model of CVA6 starts logging from the pc value of 0x80000004, hence the tests are failing in comparison of spike and model results, so either spike or verilator trace python script of CVA6 have to be updated -

That's why I have updated the spike end address of trampoline according to the end address of the verilator trampoline

@fatimasaleem
Copy link
Contributor Author

As said on previous commit, could you confirm that all the existing tests are runnable with this newest verilator?

All the tests are passing with the new version of the verilator. But if required, we can keep the older version(4.110) as it is for the time being.

@ASintzoff
Copy link
Contributor

Why are you changing the end address of trampoline in spike?

As also mentioned in the issue,

While logging the results of the spike and verilated model of CVA6, there is a mismatch of one instruction, i.e. the spike starts logging from pc value of 0x80000000 and the verilated model of CVA6 starts logging from the pc value of 0x80000004, hence the tests are failing in comparison of spike and model results, so either spike or verilator trace python script of CVA6 have to be updated -

That's why I have updated the spike end address of trampoline according to the end address of the verilator trampoline

Thanks to highlight this issue in the conversion scripts. After looking at them, I think the root cause is in the verilator script. This script is not writing executed instruction at 0x8000_0000 from .log file into .csv one. It will be better to fix the verilator script to be sure we don't drop 0x8000_0000 address.

@fatimasaleem
Copy link
Contributor Author

As link.ld is used by all tests for cva6, I'm wondering if they are still buildable and executable.

Are you facing any trouble with the previous version? What is the source of the version you propose?

Yes, I was facing a problem with compiling the test with the spike using the older version of link.ld, it was mainly because the spike linker has been updated with the new entry point but riscv-dv linker is an outdated version.

@fatimasaleem fatimasaleem changed the title [Issue-1252] Update core-v-verif to support latest riscv-arch-tests infrastructure [Issue-1252] Added the support to run latest riscv-arch-tests in core-v-verif CVA6 infrastructure Apr 25, 2022
@fatimasaleem
Copy link
Contributor Author

@ASintzoff I have made all the required changes, are there still some changes you would like to mention; which are blocking this PR? Or can we merge this PR in cv6/dev? cc @MikeOpenHWGroup

@ASintzoff
Copy link
Contributor

@ASintzoff I have made all the required changes, are there still some changes you would like to mention; which are blocking this PR? Or can we merge this PR in cv6/dev? cc @MikeOpenHWGroup

Some comments:

  • It seems Spike is not built when using dv-riscv-arch-test.sh script (only git clone is done by install-riscv-isa-sim.sh script).
  • The new linker script prevents correct builds of riscv-tests test cases as this test suite has no rvtest_entry_point. A binary is built but as the entry point is incorrect, therefore execution does not start at the correct place.

@fatimasaleem
Copy link
Contributor Author

 It seems Spike is not built when using dv-riscv-arch-test.sh script (only git clone is done by install-riscv-isa-sim.sh script).

Yes, I added install-riscv-isa-sim.sh script to only clone riscv-isa-sim directory to use the latest model_test.h source file, which was previously being used from the riscv-target folder of riscv-arch-tests repo, but in riscof-dev the riscv-target has been removed, and for using the spike simulator as an architectural test model we needed the required dependency file.

I can also invoke the build of spike as well in the same script when the $SPIKE env variable is not set.

@fatimasaleem
Copy link
Contributor Author

fatimasaleem commented May 6, 2022

The new linker script prevents correct builds of riscv-tests test cases as this test suite has no rvtest_entry_point. A binary is built but as the entry point is incorrect, therefore execution does not start at the correct place.

As, I have already mentioned above that

the same tests have been modified in the riscv-arch-test, and the older version of the test infrastructure is no longer compatible with the spike current infrastructure.

And the new test suite (riscof-dev infrastructure) is using the rvtest_entry_point instead of _start, without getting the proper entry point the spike cannot differentiate between the pc for DATA_BEGIN section and CODE_BEGIN section.

We clearly cannot maintain two different linkers for spike at the same time. If we are to migrate to newer (riscof) infrastructure then I think we need to use the latest linker entry point as it is being used by the riscof framework, it will replace the current framework of riscv-arch-test soon. I am open to any other suggestions, that how can we maintain the older linker version.

@MikeOpenHWGroup
Copy link
Member

If we are to migrate to newer (riscof) infrastructure then I think we need to use the latest linker entry point as it is being used by the riscof framework, it will replace the current framework of riscv-arch-test soon. I am open to any other suggestions, that how can we maintain the older linker version.

I am not in favour of changing our testbench infrastructure to use the RISCOF framework. The proper solution here is to create a framework in which we can select things like the linker entry points on a per test-program basis. The CV32E4 testbenches can do this, and CVA6 should adopt something like that (see slide 15 of the Writing Tests for CORE-V-VERIF presentation I presented a couple of weeks ago).

@fatimasaleem
Copy link
Contributor Author

The new linker script prevents correct builds of riscv-tests test cases as this test suite has no rvtest_entry_point. A binary is built but as the entry point is incorrect, therefore execution does not start at the correct place

@ASintzoff please check that the riscv-tests test cases are now working correctly with the latest changes.

@fatimasaleem
Copy link
Contributor Author

@ASintzoff I am not changing the existing linker anymore but added an argument in the cva6.py through which I have an option to select a different linker and by default, things can be run as they were running before.

Please let me know if still there is anything that needs more modification or addition. cc @MikeOpenHWGroup

@fatimasaleem
Copy link
Contributor Author

Pinging for merge approval.

Copy link
Contributor

@ASintzoff ASintzoff left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and all the modifications you did based on my comments.
I have just a new one on your latest commit.

cva6/regress/dv-riscv-arch-test.sh Outdated Show resolved Hide resolved
cva6/regress/install-cva6.sh Outdated Show resolved Hide resolved
@JeanRochCoulon
Copy link
Contributor

Thank you again @fatimasaleem for this contribution. For your information, we plan to upgrade the GCC version to version 12 to support the Bit Manip extension. And the arch-test will be inserted in the Thales CI. Very nice !!

@JeanRochCoulon JeanRochCoulon merged commit d0645ca into openhwgroup:cva6/dev Jul 11, 2022
@fatimasaleem
Copy link
Contributor Author

Thank you again @fatimasaleem for this contribution. For your information, we plan to upgrade the GCC version to version 12 to support the Bit Manip extension. And the arch-test will be inserted in the Thales CI. Very nice !!

Great! That's good to hear, thank you @JeanRochCoulon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants