Skip to content

pushing pmpsm and pmpzca tests for ACT4#948

Open
hamza-1821 wants to merge 24 commits intoriscv:act4from
hamza-1821:act4-work
Open

pushing pmpsm and pmpzca tests for ACT4#948
hamza-1821 wants to merge 24 commits intoriscv:act4from
hamza-1821:act4-work

Conversation

@hamza-1821
Copy link
Contributor

@hamza-1821 hamza-1821 commented Feb 18, 2026

This PR has both rv32 and rv64 tests for pmpsm and pmpzca

@hamza-1821 hamza-1821 changed the title pushing pmpsm and pmpzca tests pushing pmpsm and pmpzca tests for ACT$ Feb 18, 2026
@hamza-1821 hamza-1821 changed the title pushing pmpsm and pmpzca tests for ACT$ pushing pmpsm and pmpzca tests for ACT4 Feb 18, 2026
@jordancarlin jordancarlin added the ACT4 Issues or PRs applicable to ACT4 label Feb 20, 2026
Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

Beyond the labels that need to be updated, it looks like there are a few outstanding issues with the configuration settings.

Also, have you checked coverage with this version of the tests (by running make coverage --jobs $(nproc) and checking the report in work/sail-rv{32/64}-max/reports)? It looks like right now it won't run the PMP coverage because the directory name doesn't match the coverage file. The PMPSm tests need to be in a PMPSm directory so they get run against the PMPSm covergroups. Same for PMPZca tests in a PMPZca directory.

@UmerShahidengr
Copy link
Collaborator

@hamza-1821 you are getting a conflict, and CI is still failing

@hamza-1821
Copy link
Contributor Author

@hamza-1821 you are getting a conflict, and CI is still failing

yes, haven't pushed the latest changes yet. That will resolve CI issue. Still working on it.

jordancarlin and others added 7 commits February 28, 2026 14:17
At this point, the repo holds a number of different projects that all
require different people to review things. To make organizing this
easier, add issue templates that automatically apply labels for common
types of issues:
- CTP bugs/suggestions
- CRD bugs/suggestions
- Test framework bugs
- Test bugs
- Feature requests

---------

Signed-off-by: Jordan Carlin <jordanmcarlin@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Umer Shahid <umer.shahid@10xengineers.ai>
Co-authored-by: Umer Shahid <umer.shahid@10xengineers.ai>
Signed-off-by: Hamza <humza.ali1821@gmail.com>
Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

Still haven't reviewed most of the tests, but here are more structural comments. Also still need the test directory structure to be recategorized so that coverage works.

// attempt to lw and sw to each standard region. Read should succeed and
// write should fail, proving all PMP entries are usable.
//
// Dependencies: model_test.h, arch_test.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

This list of dependencies is outdated. Rather than trying to keep it up to date, we're probably better off just dropping it. All of the tests have the same dependencies.

//
// Notes : - Ensure `rvmodel_macros.h` is configured to match your PMP grain
// and PMP region count before running this test.
// - Adjust PMP configurations for specific hardware if required.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this note mean and how does it differ from the one above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the older version, there were some tests requiring pmp grain to be 0. we had checks in regex for pmp grain and pmp count for the test to be selected, and this note gives you a hint that configurations should be matched. We can remove this note in the next commit.

@@ -0,0 +1,301 @@
// --------------------------------------------------------------------------------
// Title : Comprehensive PMP (Physical Memory Protection) Verification
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file (and all of the PMP tests) are missing license headers. Please add an appropriate license. Most of the other tests use Apache 2.

// Set up test environment and declare main entry point
RVTEST_BEGIN

#define NOP 0x13
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is needed, should we move it to a more central location? Maybe utils.h. Right now it is defined in all of these tests.


.macro VERIFICATION_RWX ADDRESS, TEST_CASE

LA(a5, \ADDRESS) // Address to be ve
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment unclear.

Comment on lines 52 to 58
#ifndef RVMODEL_PMP_GRAIN
#define RVMODEL_PMP_GRAIN 4
#endif

#ifndef RVMODEL_NUM_PMPS
#define RVMODEL_NUM_PMPS 64
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this was removed from some but not all of the tests. Please double check to make sure it is removed from all of the tests since we are going to require the user to always define this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be entirely removed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of splitting pmpsm_all_entries_check into 4 parts? Seems like it would be easier to understand what each test does if this was kept together as a single test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was split into 4 parts most probably beacause of the size of the signature region being too large for a single test. We can definitely use a single test that tests the upper pmp entries.

Comment on lines +81 to +85
LI(x4, (NA4 << PMP3_CFG_SHIFT|NA4 << PMP2_CFG_SHIFT|NA4 << PMP1_CFG_SHIFT|NA4 << PMP0_CFG_SHIFT))
csrw pmpcfg0, x4
csrw pmpcfg1, x4
csrw pmpcfg2, x4
csrw pmpcfg3, x4
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test seems to be missing any kind of self-checking. We need to read back what we wrote to each CSR to make sure it holds the proper value. Look at the RVTEST_SIGUPD_CSR_WRITE macro in signature.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please double check all of the tests. There are likely other parts that are missing self-checking beyond this one too.


// Test case strings for reporting
canary_mismatch: .string "Testcase signature canary mismatch!"
test_1_str: .string "\"test: 1; cp: cp_pmpm64_lw\""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine to defer this to a follow-up PR after this one is merged, but the test strings should include the covergroup and bin in addition just the coverpoint. That will probably require the VERIFICATION_RWX macro to include the TEST_CASE number argument as part of the debug string (similar to what is already done for the labels).

Comment on lines 27 to 41
# REQUIRED_EXTENSIONS: ['I','Zca','S','Smpmp']
# params:
# MXLEN: 32
# MARCH: rv32ic_zicsr
# CONFIG_DEPENDENT: true
##### END_TEST_CONFIG #####

// Define the name of this test file for reporting and the size of the signature region
#define TEST_FILE "pmpzca_aligned_tor.S"
#define SIGUPD_COUNT 50

// Enable trap handler
#define rvtest_mtrap_routine
#define RVTEST_PRIV_TEST
#define rvtest_strap_routine
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do the Zca tests still require S and the rvtest_strap_routine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might have missed the removal of strap handler from zca tests, and some sm tests. Since these tests only run in M mode, i will remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ACT4 Issues or PRs applicable to ACT4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants