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

Use better types for virtual memory translation code #637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Dec 6, 2024

Change the VM code from using bits(64) everywhere to using more accurate types. This also enables Sv57 support.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Dec 6, 2024

I wrote this code quite a while ago and kind of got sidetracked by other stuff, but it looks like I finished it so I thought I may as well push a draft and see what people think. It hasn't been tested extensively yet but I can do that.

One slightly awkward thing I found is that you can use really nice strict types everywhere for the actual translation because the lifetime of sv_width is the scope of the translation function, so it can only have one value. But you can't really do it for the TLB because the lifetime of sv_width for the TLB is infinite so you can end up storing Sv39 addresses in it one cycle and the Sv57 a few cycles later. I just used the widths for Sv57 (the max) and then truncate/zero_extend as appropriate.

Copy link

github-actions bot commented Dec 6, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit da2538a. ± Comparison against base commit 601f3d8.

♻️ This comment has been updated with latest results.

@Timmmm Timmmm force-pushed the user/timh/ptw_nice_types branch from 55e080e to 8c15f7c Compare December 10, 2024 10:21
@Timmmm Timmmm force-pushed the user/timh/ptw_nice_types branch from 8c15f7c to dcb8d5b Compare December 19, 2024 11:23
@Timmmm Timmmm marked this pull request as ready for review January 2, 2025 10:58
@Timmmm
Copy link
Collaborator Author

Timmmm commented Jan 2, 2025

Ok I think this is ready. I will run it through our CI system and report the test results.

@Timmmm Timmmm force-pushed the user/timh/ptw_nice_types branch from dcb8d5b to 079b1a7 Compare January 2, 2025 11:48
Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I have not reviewed this super thoroughly and am probably not the best person to review the vmem code but it looks good to me.

model/riscv_vmem_tlb.sail Outdated Show resolved Hide resolved
model/riscv_vmem_tlb.sail Outdated Show resolved Hide resolved
@Timmmm Timmmm force-pushed the user/timh/ptw_nice_types branch from 559d67b to 4063c08 Compare January 3, 2025 09:03
Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

LGTM but someone else should also take a look

Change the VM code from using `bits(64)` everywhere to using more accurate types. This also enables Sv57 support.
@Timmmm Timmmm force-pushed the user/timh/ptw_nice_types branch from 4063c08 to da2538a Compare January 9, 2025 16:18
// and does not legalize the asid field if asidlen is not the maximum.
function legalize_satp(
a : Architecture,
o : xlenbits,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest adding comment: /* previous value of satp */

function legalize_satp(
a : Architecture,
o : xlenbits,
v : xlenbits,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest adding comment: /* proposed new value of satp */


// PUBLIC: invoked in init_vmem() [riscv_vmem.sail]
function init_TLB() -> unit = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we not need 'init_TLB()' if we allow a dynamic reset on the Sail model?

@@ -127,7 +127,6 @@ function loop () : unit -> unit = {
function init_model () -> unit = {
init_platform (); /* devices */
init_sys (); /* processor */
init_vmem (); /* virtual memory */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we not need 'init_vmem()' if we allow a dynamic reset on the Sail model?

Copy link
Collaborator

@rsnikhil rsnikhil left a comment

Choose a reason for hiding this comment

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

I have reviewed all 15 changed files; all generally LGTM.

Some very minor suggestions (see individual comments on particular files/lines).

I did not study the translation algorithm in riscv_vmem.sail in detail; it looked similar (structure, flow) to original, just fixing it up for the new tighter vmem types, so I think it's probably fine.

The previous version was less type-safe, using 64 bits everywhere, but could work for dynamic choice between Sv32, Sv39, Sv48 and Sv57. I think perhaps this new code requires this to be a static choice, to enable type-checking?

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.

3 participants