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

Add svnapot extention support #510

Closed
wants to merge 5 commits into from
Closed

Add svnapot extention support #510

wants to merge 5 commits into from

Conversation

Yui5427
Copy link

@Yui5427 Yui5427 commented Jul 4, 2024

No description provided.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

This looks like it allows any NAPOT size, whereas the spec currently only allows 64kB ones.

I think the bit manipulation can probably be simplified too but I'd have to think about that.

@@ -45,6 +45,15 @@ function vpn_j_of_va(sv_params : SV_Params,
((va >> lsb) & mask)
}

// PRIVATE: Count trailing zeros
function LowestSetBit(x : bits(64)) -> bits(64) = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Funnily enough I have a version of this already that I wrote to demonstrate Sail's fancy type checking:

val count_trailing_zeros : forall 'n, 'n >= 0 . (bits('n)) -> range(0, 'n)
function count_trailing_zeros(x) = {
    foreach (i from 0 to ('n - 1)) {
        if x[i] == bitone then return i
    };
    'n
}

However I'm not sure you need this at all - if you check the PMP code it does some clever bit manipulation instead.

model/riscv_vmem.sail Outdated Show resolved Hide resolved
@Yui5427
Copy link
Author

Yui5427 commented Jul 4, 2024

I check the code in PMP,It really lights me,And I think the additional code would solve the problem.It adds the 64K check.And use the PMP napot bit manipulation to count the napot_mask. And replaced "perfect_one" with zero_extend(0b1).lol :)

// For svnapot ext purpose.
if (pte & (zero_extend(0b1) << 63)) == (zero_extend(0b1) << 63) then {
if (napot_mask != zero_extend(0xf)) then {
return PTW_Failure(PTW_Invalid_PTE(), ext_ptw)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean PTW_Unsupported_Napot here?

I'd probably call it PTW_Reserved_Napot btw.


// For svnapot ext purpose.
if (pte & (zero_extend(0b1) << 63)) == (zero_extend(0b1) << 63) then {
if (napot_mask != zero_extend(0xf)) then {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Easier like this: napot_mask[3 .. 0] != ones()

@@ -164,7 +164,30 @@ function pt_walk(sv_params,
}
}
else {
let mask_bits = level * sv_params.pte_PPN_j_size_bits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't gone through the logic properly but are you sure this is right? level is guaranteed to be 0 here.

@Timmmm
Copy link
Collaborator

Timmmm commented Jul 4, 2024

You also need to update pte_is_invalid() to check the N bit, and add a haveSvnapot() function.

@ved-rivos
Copy link
Contributor

Svnapot was queued by an earlier PR #393
Its unfortunate that the PR backlog is leading to duplicate work.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 5, 2024

Svnapot needs to not have an effect when not enabled, so that the bits can be used by another extension. See Svpbmt for a similar case.

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.

Needs to be opt-in/out-able

@Timmmm
Copy link
Collaborator

Timmmm commented Jul 5, 2024

Svnapot was queued by an earlier PR #393
Its unfortunate that the PR backlog is leading to duplicate work.

I agree, sorry I forgot about your PR. I think we should close this one in favour of yours which is much more complete.

@Yui5427 Yui5427 closed this Jul 8, 2024
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.

4 participants