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

Physical Memory Protection (32/64) Tests and Covergroups #462

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

MuhammadHammad001
Copy link
Contributor

Description

This PR adds the arch tests for the PMP for 32 bit and 64 bit architecture and also the Covergroups have been added as well for the coverage analysis. This implements the Physical Memory Protection (PMP) section from the following Test Plan. The assembly arch tests have been developed by Mr. Umer Shahid and the coverpoints are developed by Muhammad Hammad Bashir.

Related Issues

The links to the PRs that need to be merged before this PR:

Ratified/Unratified Extensions

  • Ratified
  • Unratified

List Extensions

Privileged Architecture -- Physical Memory Protection

Reference Model Used

  • SAIL
  • Spike
  • Other - < SPECIFY HERE >

Mandatory Checklist:

  • All tests are compliant with the test-format spec present in this repo ?
  • Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
  • Ran the new tests on RISCOF in coverage mode
  • Link to Google-Drive folder containing the new coverage reports available at: < Coverage Reports PMP >
  • Link to PR in RISCV-ISAC from which the reports were generated : < PR #80 -- Support for Privileged Architecture in RISC-V ISAC>
  • Changelog entry created with a minor patch

Optional Checklist:

  • RISCV-V CTG PR link if tests were generated using it : < SPECIFY HERE >
  • Were the tests hand-written
  • Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description
  • If you have modified arch_test.h Please provide a detailed description of the changes in the Description section above.

@MuhammadHammad001
Copy link
Contributor Author

@allenjbaum @jjscheel

@jjscheel jjscheel mentioned this pull request Apr 30, 2024
13 tasks
@jamesbeyond jamesbeyond added the ratified ratified specs label May 20, 2024
@jamesbeyond jamesbeyond changed the base branch from main to dev May 28, 2024 16:53
@jamesbeyond jamesbeyond added the size-L Large efforts required label Jun 6, 2024
@MuhammadHammad001
Copy link
Contributor Author

@allenjbaum Can you please review this version of coverpoints and the tests? The updated coverage report has also been attached above for your reference.

@UmerShahidengr
@jamesbeyond

Signed-off-by: Muhammad Hammad Bashir <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
@jamesbeyond
Copy link
Collaborator

@MuhammadHammad001 , could have look at the CI checks, seems those added tests are failing

@MuhammadHammad001
Copy link
Contributor Author

@jamesbeyond The reason the CI is failing on the tests is because as mentioned in the description, these tests and covergroups are written following the new privilege architecture features support in RISC-V ISAC and RISCOF. Once, those PRs are merged, these tests will also pass the CI.

Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

I'm a little concerned that the helper macros are hardcoded as to the PMP registers numbers, Is there any way those can be passed in as a parameter? that will help other tests n the future

coverage/rv32_pmp.cgf Outdated Show resolved Hide resolved
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

Overall the test structure is for each mode, different RWX settings with R,W, & X ops (R,W,RW,RX,RWX) or priority cases with same settings & a lower priority with RWX settings, or
a 3 level priority with the middle being no allowed accesses.
What is missing are for the first 2 cases: a noaccess case, for the second RWX cases (which is OK because its redundant).
In all case tests, the restricted case is higher priority than the all or none case.
I think you also need need tests where a restricted case is lower priority than no_Access or all access cases (see also comments on the first 2 tests).
I don't think there are tests here where the access is not matched by any entry, or paritally matched by any entry (without being unaligned). I think you can only do that with NA4 &ld, sd for RV64, and FLD,FSD in RV32 (so F-ext must be supported in that case)

Updated pmp_cfg_locked_write_unrelated coverpoint based on Allen comment

Signed-off-by: Umer Shahid <[email protected]>
allenjbaum
allenjbaum previously approved these changes Aug 2, 2024
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

Bug is fixed

Updated pmp_cfg_locked_write_unrelated coverpoint in rv64 to update a minor bug

Signed-off-by: Umer Shahid <[email protected]>
allenjbaum
allenjbaum previously approved these changes Aug 2, 2024
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

The RV64 bugs fixed

@UmerShahidengr
Copy link
Collaborator

@allenjbaum please approve this PR again, your previous approval was dismissed because of the new commits, now it is passing the CI, so it is good to go now.

@jordancarlin
Copy link
Contributor

Looks like the riscof_work folder was added in the latest commit. This definitely shouldn’t be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privileged ratified ratified specs size-L Large efforts required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants