Add read-only CSR MCONFIGPTR

[dansmathers]

CSR MCONFIGPTR is defined in RISCV priv spec 1.12 but is missing from the RISC-V SAIL model. This PR adds the read-only CSR MCONFIGPTR.

[jrtc27]

1. Commit subject isn't a proper subject; should start with a capitalised word
2. Verbs describing what the commit message does should be in the imperative mood, i.e. "Add" not "Adding" (see https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches?h=v2.36.1#n181 for example which documents good practice)
3. CSR should be capitalised
4. The TG link isn't so helpful; what is more helpful is stating that it's a new CSR added in Priv 1.12

The diff itself looks fine

[github-actions]

Unit Test Results

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

Results for commit fbb7edc. ± Comparison against base commit 58cac61.

[dansmathers]

Hi, anything else I need to do to get this merged into sail? FYI @billmcspadden-riscv

[jrtc27]

You haven't done anything to the commit. The PR subject and body is not the same as the commit subject and body.

[jrtc27]

And you still reference the TG in the PR's message, which isn't meaningful. It being used for unified discover is, and it being in Priv 1.12 for that purpose is, but the TG is meaningless. It's just a group of people that form an organisational structure. It's like saying "CSR fflags is being used for floating point by the unprivileged architecture standing committee".

[dansmathers]

Ok. Updated. ok to merge?

[billmcspadden-riscv]

Hi Dan. We'll discuss it in tomorrow's golden model meeting. Bill Mc.

…

On Mon, Aug 21, 2023 at 5:25 PM Dan Smathers ***@***.***> wrote: Ok. Updated. ok to merge? — Reply to this email directly, view it on GitHub <#293 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXROLOHXEIEHF42ROQE5B6DXWPN6HANCNFSM6AAAAAA3THEDKM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Bill McSpadden Formal Verification Engineer RISC-V International mobile: 503-807-9309

[jrtc27]

Ok. Updated. ok to merge?

No it's not. The commit is still unchanged. Please fix the commit message like I said.

[dansmathers]

I don't see an easy way to do that using the github gui. Feel free to edit.
Thanks

[jrtc27]

Correct, there is not. You should be using a Git client, because the GitHub web interface is extremely limited. This admission is also rather concerning since, if you were using the web interface to create this PR, that means you cannot possibly have tested it locally, right?

[dansmathers]

It was tested against sail and spike using riscof.
riscv-non-isa/riscv-arch-test#377

[jrtc27]

Then use your Git client with the repository you cloned for RISCOF? I don't understand why the web GUI is a problem in that case.

[jrtc27]

It was tested against sail and spike using riscof.

Also surely you mean just Spike there, given you're editing the Sail model...

[billmcspadden-riscv]

This looks ok to me.

[jrtc27]

Blocking on getting an appropriate commit message

[billmcspadden-riscv]

The instantiation of the CSR looks correct to me. However, two issues need to be addressed.

1. How will this register be initialized? I suggest a command line argument be added to
   supply the value to the register. This code can be found in c_emulator/riscv_sim.c I can help with the implementation of the command line switch.
2. The priv spec has verbiage concerning the legal value contained in the CSR. Here's the pertinent text:
   The pointer alignment in bits must be no smaller than the greatest supported MXLEN: i.e., if the
   greatest supported MXLEN is 8 × n, then mconfigptr[log2 n-1:0] must be zero.

This must be checked. I think it would be acceptable to check the value as you are processing the command line argument.

[Timmmm]

I think this should be merged as is (after the commit message is changed).

How will this register be initialized?

There are plenty of similar registers that do not have a way of setting them yet (mhartid most egregiously), so I don't think this should block that.

I think the alignment checking can be done at a later date too. (The spec does not say what should happen if the pointer is not appropriately aligned anyway, so it's unclear what the model should do - if anything.)

[Timmmm]

@dansmathers did you mean to close this?

[dansmathers]

yes. I had created it with a fork instead of a branch so my understanding it was preventing me creating other branches. not sure it was worth dealing with commit message corrections. it's easy enough for someone else to implement.

[Alasdair]

If it helps, for future reference, before making a PR, you can do git checkout -b to move to a new branch, or you can commit from your local master branch to a different remote branch on github using git push <remote> master:<new_remote_branch>, then make a PR from that remote branch.

Updating a commit message is either git commit --amend for the last commit, or git rebase --interactive to edit multiple previous commits. Then you can force push to your remote branch that was made for the PR (which is why it's best to make a new branch for each PR, as you don't usually want to force push to master).

[Alasdair]

If you like I can re-create your commit with an updated commit message

[dansmathers]

It's just simple additions to the following files:

riscv_csr_map.sail:mapping clause csr_name_map = 0xF15 <-> "mconfigptr"
riscv_insts_zicsr.sail: (0xF15, _) => mconfigptr,
riscv_sys_control.sail: 0xf15 => p == Machine, // mconfigptr
riscv_sys_control.sail: mconfigptr = zero_extend(0b0);
riscv_sys_regs.sail:register mconfigptr : xlenbits

thanks for offering to assist.

[Alasdair]

Ok, I've opened a new pull request here #422 the contents should be the same I think