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

Added support of newer additional PMP configuration and address CSRs #168

Closed
wants to merge 1 commit into from

Conversation

Abdulwadoodd
Copy link

This PR contains all the PMP configuration and address CSRs to support 64 PMP entries as defined by the spec

@Abdulwadoodd
Copy link
Author

Hi @martinberger @jrtc27,
Kindly review the PR

Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

Haven't methodically checked for copy/paste errors but this is what I found skimming it.

I had some ideas a while back about how to reduce the code duplication, but I don't think that should block adding this support, even if it gets rather silly.

Comment on lines 139 to 142
mapping clause csr_name_map = 0x3A0 <-> "pmpcfg0"
mapping clause csr_name_map = 0x3A1 <-> "pmpcfg1"
mapping clause csr_name_map = 0x3A2 <-> "pmpcfg2"
mapping clause csr_name_map = 0x3A3 <-> "pmpcfg3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mapping clause csr_name_map = 0x3A0 <-> "pmpcfg0"
mapping clause csr_name_map = 0x3A1 <-> "pmpcfg1"
mapping clause csr_name_map = 0x3A2 <-> "pmpcfg2"
mapping clause csr_name_map = 0x3A3 <-> "pmpcfg3"

(0x3A7, 32) => { pmpWriteCfgReg(7, value); Some(pmpReadCfgReg(7)) }, // pmpcfg7,
(0x3A8, _) => { pmpWriteCfgReg(8, value); Some(pmpReadCfgReg(8)) }, // pmpcfg8,
(0x3A9, 32) => { pmpWriteCfgReg(9, value); Some(pmpReadCfgReg(9)) }, // pmpcfg9,
(0x3AA, _) => { pmpWriteCfgReg(10, value); Some(pmpReadCfgReg(10))}, // pmpcfg10,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need a space before the }

model/riscv_pmp_control.sail Outdated Show resolved Hide resolved
0x3ED => p == Machine, // pmpaddr61
0x3EE => p == Machine, // pmpaddr62
0x3EF => p == Machine, // pmpaddr63

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

(0x3AD, 32) => { pmpWriteCfgReg(13, value); Some(pmpReadCfgReg(13))}, // pmpcfg13,
(0x3AE, _) => { pmpWriteCfgReg(14, value); Some(pmpReadCfgReg(14))}, // pmpcfg14,
(0x3AF, 32) => { pmpWriteCfgReg(15, value); Some(pmpReadCfgReg(15))}, // pmpcfg15,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

mapping clause csr_name_map = 0x3ED <-> "pmpaddr61"
mapping clause csr_name_map = 0x3EE <-> "pmpaddr62"
mapping clause csr_name_map = 0x3EF <-> "pmpaddr63"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@github-actions
Copy link

github-actions bot commented Jun 29, 2022

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit cf411dc. ± Comparison against base commit 5fce6fb.

♻️ This comment has been updated with latest results.

@Abdulwadoodd
Copy link
Author

Resolved the formatting issues, empty lines and duplications

@Abdulwadoodd
Copy link
Author

Abdulwadoodd commented Jul 14, 2022

Hello @jrtc27 @martinberger I have tested the updated SAIL model by writing tests that contain pmpcfg[4,15] and pmpaddr[16,63]. And SAIL is giving the expected result. Is there anything else to do before merging this PR?

@scottj97
Copy link
Contributor

@Abdulwadoodd please squash the fixes back into the first commit, instead of adding a new commit to clean up the earlier one.

@Abdulwadoodd
Copy link
Author

Hi @scottj97, I have squashed the changes into a single (earlier) commit.

Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

Commit message is wrong, PMP configuration and address CSRs already exist, just not the newer additional ones

Comment on lines 413 to 414
Machine => true,
_ => false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't reformat this

Copy link
Author

Choose a reason for hiding this comment

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

Done

pmp17cfg = pmpWriteCfg(pmp17cfg, v[15..8]);
pmp18cfg = pmpWriteCfg(pmp18cfg, v[23..16]);
pmp19cfg = pmpWriteCfg(pmp19cfg, v[31..24]);
pmp20cfg = pmpWriteCfg(pmp19cfg, v[39..32]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lots of copy/paste errors in these

Copy link
Author

Choose a reason for hiding this comment

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

FIxed

@Abdulwadoodd
Copy link
Author

Commit message updated

@Abdulwadoodd Abdulwadoodd changed the title Added support of PMP configuration and address CSRs Added support of newer additional PMP configuration and address CSRs Sep 2, 2022
@ptomsich
Copy link
Collaborator

This PR has no activity for almost 9 months.
Please help to update its status:

  • @jrtc27 Have the style issues been sufficiently addressed?
  • @Abdulwadoodd Please rebase onto master, so this applies without conflicts!

@jrtc27
Copy link
Collaborator

jrtc27 commented May 29, 2023

Style looks fine

@ptomsich ptomsich added tgmm-agenda Tagged for the next Golden Model meeting agenda. and removed tgmm-agenda Tagged for the next Golden Model meeting agenda. labels May 29, 2023
@ptomsich
Copy link
Collaborator

Open questions from the golden-model meeting:

  • @Abdulwadoodd Are you still maintaining this PR? Can you provide an ETA for rebasing this?

@Abdulwadoodd
Copy link
Author

Abdulwadoodd commented May 30, 2023

@ptomsich, thanks for the reminder. I will rebase it by 2nd June.

@ptomsich
Copy link
Collaborator

@Abdulwadoodd Thanks for the update! I'll keep this in the queue, so we can move it forward shortly thereafter.
@billmcspadden-riscv As discussed, please review the new version (once it comes in) for correctness relative to the specification!

@martinberger
Copy link
Collaborator

I will rebase it by 2nd June.

Hello @Abdulwadoodd , any updates on this?

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 23, 2023

Since the PMP CSR addresses are nicely aligned, you can somewhat reduce the crazy amount of copy and pasting like this:

readCSR:
    (0b001111 @ idx : bits(6),  _) => pmpReadAddrReg(unsigned(idx)),
...
register pmpaddr  : vec(64, dec, xlenbits)
...
function pmpReadAddrReg(n : range(0, 63)) -> xlenbits = {
   ...
}

(pmpReadAddrReg is needed anyway to support PMP grain, which we have implemented an plan to upstream.)

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 23, 2023

In fact maybe hold off on this. I have implemented the approach I mentioned above which means adding support for 64 PMPs is a 1 line change instead of a gazillion line copy/paste. I'll try to send a PR soon.

@Abdulwadoodd
Copy link
Author

It is completed by #350.

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.

6 participants