Replace old virtual memory code with new (details follow)

[rsnikhil]

[ This PR replaces old PR #365 on same topic, which was closed]

Old vmem code had much 'cut-and-paste' replication for RV32 (Sv32) and RV64 (Sv39, Sv48), and was scattered over several files.

New code unifies them into single set of parameterized functions that works for RV32/RV64 and Sv32/Sv39/Sv48 (and is ready for Sv57).

Deleted old files:
riscv_vmem_rv32.sail riscv_vmem_rv64.sail
riscv_vmem_sv32.sail riscv_vmem_sv39.sail riscv_vmem_sv48.sail
riscv_pte.sail
riscv_ptw.sail

Current files: all named riscv_vmem_*
riscv_vmem.sail (root file for vmem)
riscv_vmem_common.sail
riscv_vmem_pte.sail
riscv_vmem_ptw.sail
riscv_vmem_tlb.sail
riscv_vmem_types.sail

Modified top-level Makefile accordingly.

Added documentation on new vmem code: doc/notes_Virtual_Memory.adoc

[github-actions]

Test Results

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

Results for commit a0099b7. ± Comparison against base commit d564b93.

♻️ This comment has been updated with latest results.

[Timmmm]

Hi, I finally managed to do some testing of this. Didn't quite do as much as I wanted but I ran it through our CI system and it passed all the tests (about 100 tests from various sources; not all VM focused but lots of them do randomly test it).

Can you just rebase & squash the commits and make the commit message subject into the normal verb tense:

Unify virtual memory code

Old vmem code had much 'cut-and-paste' replication for RV32 (Sv32) and
RV64 (Sv39, Sv48), and was scattered over several files.

New code unifies them into single set of parameterized functions
that works for RV32/RV64 and Sv32/Sv39/Sv48 (and is ready for Sv57).

Deleted old files:
    riscv_vmem_rv32.sail    riscv_vmem_rv64.sail
    riscv_vmem_sv32.sail    riscv_vmem_sv39.sail    riscv_vmem_sv48.sail
    riscv_pte.sail
    riscv_ptw.sail

Current files: all named riscv_vmem_*
    riscv_vmem.sail    (root file for vmem)
    riscv_vmem_common.sail
    riscv_vmem_pte.sail
    riscv_vmem_ptw.sail
    riscv_vmem_tlb.sail
    riscv_vmem_types.sail

Modified top-level Makefile accordingly.

Added documentation on new vmem code: doc/notes_Virtual_Memory.adoc

Easy way to do this if you aren't a git pro:

git fetch  # Update origin/master
git switch <this branch>
git merge origin/master  # Merge in origin/master changes so the only difference with origin/master is your change.
git reset --soft origin/master   # Move the branch pointer to origin/master, while retaining the working tree and staging the changed files.
git commit   # Create the commit again.
<paste in the commit message>
git push -f

[rsnikhil]

Hi, I finally managed to do some testing of this.

Thanks very much!

Can you just rebase & squash the commits and make the commit message subject into the normal verb tense:
...
Easy way to do this if you aren't a git pro:

I’m not a git pro; I tried the commands you suggested, and spent several hours trying several variants, without success.

The current (top of) the git graph in https://github.com/rsnikhil/sail-riscv is:

*   a0099b7 (HEAD -> master, origin/master, origin/HEAD) Merge branch 'riscv:master' into master
|\  
| * d564b93 Fix MEnvCall value
* | 932469c Merge branch 'riscv:master' into master
|\| 
| * 51b9732 Fix error in senvcfg definition
| * 4a7c815 Fix typo in E_SAMO_Addr_Align description
| * 4211935 Fix error in format specifiers
| * df6eac3 Replace atom with int
| * fd21acc Add RV32 restriction for compressed shift instructions
* | 4bee248 Merge branch 'riscv:master' into master
|\| 
| * c287c34 Add missing comparison operators
| * baafe6f CI: Update upload-artifact action to v4
* | 9ccf1d5 Old vmem code had much 'cut-and-paste' ...
|/  
* 9602e3a Add m/senvcfg to CSR name map

I assume we want the following result (i.e., 9ccf1d5 is rebased on top, and the three “Merge branch” commits a0099b7, 932469c and 4bee248 are “squashed” away:

*   9ccf1d5 (HEAD -> master, origin/master, origin/HEAD) Old vmem code had much 'cut-and-paste' ...
 \  
  * d564b93 Fix MEnvCall value
  * 51b9732 Fix error in senvcfg definition
  * 4a7c815 Fix typo in E_SAMO_Addr_Align description
  * 4211935 Fix error in format specifiers
  * df6eac3 Replace atom with int
  * fd21acc Add RV32 restriction for compressed shift instructions
  * c287c34 Add missing comparison operators
  * baafe6f CI: Update upload-artifact action to v4
 /  
* 9602e3a Add m/senvcfg to CSR name map

Despite hours of trying various git commands, I’ve been unable to achieve this.
I’m giving up until either some can show me how, or someone else can do it.

[UmerShahidengr]

@billmcspadden-riscv , @rsnikhil I have run all tests of sv32, sv39, and sv48, and the model is passing all tests, so this PR is ready to go.

[arichardson]

The commented out code should be removed. I thought that was fixed in the previous version of this PR?

[jrtc27]

You know, it would be nice for these long-standing pull requests to give people a heads up when you intend to merge something. An hour between one GitHub user's review (who AFAIK hasn’t got any commits in the repo?) and it being merged is a rather aggressive timeline (and rather lacking in review for something so big and critical), even more so when it’s a public holiday in many countries.

[allenjbaum]

Hmm - we should probably add something to the contributors notes about that.

…

On Mon, Apr 1, 2024 at 8:29 AM Jessica Clarke ***@***.***> wrote: You know, it would be nice for these long-standing pull requests to give people a heads up when you intend to merge something. An hour between one GitHub user (who AFAIK hasn’t got any commits in the repo?) and it being merged is a rather aggressive timeline (and rather lacking in review for something so big and critical), even more so when it’s a public holiday in many countries. — Reply to this email directly, view it on GitHub <#408 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJS2RZ4UCAPP255PDELY3F4PLAVCNFSM6AAAAABDOXAAHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRZHE3TMNZYHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Timmmm]

An hour between one GitHub user's review (who AFAIK hasn’t got any commits in the repo?) and it being merged

I don't think it was that review that actually prompted the merge (though I may be wrong) but that's a fair point - probably worth a "this will be merged in X days unless anyone has any objections" next time.

[jrtc27]

An hour between one GitHub user's review (who AFAIK hasn’t got any commits in the repo?) and it being merged

I don't think it was that review that actually prompted the merge (though I may be wrong) but that's a fair point - probably worth a "this will be merged in X days unless anyone has any objections" next time.

Even more reason to not merge it out of the blue?..

[Alasdair]

We had been discussing merging this for several weeks in our meetings (which are recorded). This was hardly out of the blue.