-
Notifications
You must be signed in to change notification settings - Fork 705
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
PMP Verif Plan and tests #2648
base: master
Are you sure you want to change the base?
PMP Verif Plan and tests #2648
Conversation
@OlivierBetschi can you rebase ? |
3e953dc
to
2ab9029
Compare
You should create a job in CI to run this test, that's a way to be sure they pass before merging. |
@valentinThomazic do you approve the DVPlan ? and in case of need, could you help @OlivierBetschi to define a new job in CI. |
✔️ successful run, report available here. |
Some comments on the test script @OlivierBetschi :
|
To get the result of the CI execution of the PMP tests directly on the PR pipeline report, please add the following job to the gitlab-ci @OlivierBetschi : pmp_tests_cv32a65x: extends: - .fe_smoke_test variables: DASHBOARD_JOB_TITLE: "[DEBUG! TO REMOVE FROM LIGHTS TESTS] PMP cv32a65x" DASHBOARD_JOB_DESCRIPTION: "PMP regression tests" DASHBOARD_SORT_INDEX: 0 DASHBOARD_JOB_CATEGORY: "Basic" DV_SIMULATORS: "vcs-uvm" SPIKE_TANDEM: 1 script: bash verif/regress/pmp_cv32a65x_tests.sh after_script: *simu_after_script |
Updated for review, but most only 2 tests are passing. I will have to debug this before the PR is merged. @valentinThomazic could you still have a look at the changes ? Thanks |
❌ failed run, report available here. |
DASHBOARD_JOB_DESCRIPTION: "PMP regression tests" | ||
DASHBOARD_SORT_INDEX: 0 | ||
DASHBOARD_JOB_CATEGORY: "Basic" | ||
DV_SIMULATORS: "vcs-uvm" |
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.
DV_SIMULATORS: "vcs-uvm" | |
DV_SIMULATORS: "vcs-uvm" | |
COLLECT_SIMU_LOGS: 1 |
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.
to have the logfiles on the dashboard
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.
Can you add "SPIKE_TANDEM: 1" to enable tandem and collect the ci results ?
verif/regress/pmp_cv32a65x_tests.sh
Outdated
@@ -0,0 +1,296 @@ | |||
# Copyright 2024 Thales DIS design services SAS |
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.
You might want to change this
verif/regress/pmp_cv32a65x_tests.sh
Outdated
# SPDX-License-Identifier: Apache-2.0 WITH SHL-2.0 | ||
# You may obtain a copy of the License at https://solderpad.org/licenses/ | ||
# | ||
# Original Author: Jean-Roch COULON - Thales |
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 should be changed
verif/sim/cva6.py
Outdated
@@ -954,6 +961,27 @@ def load_config(args, cwd): | |||
|
|||
args.spike_params = get_full_spike_param_args(args.spike_params) if args.spike_params else "" | |||
|
|||
def edit_Makefile(UVM_TESTNAME="uvmt_cva6_firmware_test_c"): |
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 it would be better to pass the uvm test to the Makefile as an env var instead of rewriting it at each simulation. This is how it is done for such things like the test path.
This is how you can do this :
- add a
uvm_test
parameter to therun_test
function incva6.py
(I admit that this function has too much parameters...) - also add it in
get_iss_cmd
function - substitute it in the cmd like it is done for the elf path for instance
- add a
uvm_test
env var for the uvm targets inverif/sim/cva6.yaml
- set a default value in
verif/sim/Makefile
for the environment varuvm_test
- replace
UVM_TESTNAME=uvmt_qqchose
withUVM_TESTNAME=$(uvm_test)
inverif/sim/Makefile
Do not hesitate to ask for more details if needed
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 guess we could call each others if you think this could speed up things
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.
@OlivierBetschi I wonder why you use a different uvm test than "uvmt_cva6_firmware_test_c" ?
@AnouarZajni, can you help us to understand what is the best solution ?
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.
The different uvm_test came from the need to disable the cache for the PMP testing (that is my understanding at least).
the uvm test is missing from the pr hence the fail |
❌ failed run, report available here. |
1 similar comment
❌ failed run, report available here. |
Hello @JeanRochCoulon @valentinThomazic , I updated the PR : all tests are now passing with vcs-uvm on my side. The change of the uvm_test was not necessary after all. Just let me know what I should update for the gitlab ci. |
@OlivierBetschi there isn't any report on the dashboard because on your latest pipeline, there isn't any comparison made between spike and the rtl simulation. I see that you disabled spike tandem, this is what causes the issue. |
…ci, update the sh script to run all the tests and add the missing parameter in cva6.py
…ry to switch to a different firmware test
83ee53b
to
23e25e6
Compare
❌ failed run, report available here. |
Hello @OlivierBetschi |
Verification Plan provided in VP_TOOL for the PMP. The verification plan should be complete, however only a partial set of the tests is available. This is not included in the CI but a bash script is available to run the test. Replacement of #2457