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

Big endianness Support added #751

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MuhammadHammad001
Copy link
Contributor

Hi @jordancarlin and @Timmmm
This PR adds the support for the big endianness in Sail RISC-V.

The test for the verification of this support is available here.

Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I left some initial comments below.

One other thing that I don't see here is the necessary modification to the reset function. Reset should set endianness back to little-endian if supported.

@@ -236,3 +236,6 @@ type max_mem_access : Int = 4096

// Type used for memory access widths. Zero byte accesses are not allowed.
type mem_access_width = range(1, max_mem_access)

// Function to reverse endianness.
val reverse_endianness = pure {c: " reverse_endianness"} : forall 'n . (bits('n * 8)) -> bits('n * 8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to just use the existing sail reverse_endianness library? https://github.com/rems-project/sail/blob/6a8d444e41a3e04798e874325bf4ec0f813e3a3a/lib/reverse_endianness.sail#L47

Just put a $include <reverse_endianness.sail> at the top of the prelude.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually tried this but was getting the following error:
image

This is because of the difference of types being passed by the function (bits(8 * 'n)) and type taken by the function bits('n).
Actually, this solution was suggested by @Timmmm and this method made the function accept values forall 'n instead of using contrained values.

Copy link

github-actions bot commented Feb 19, 2025

Test Results

392 tests  ±0   392 ✅ ±0   1m 22s ⏱️ +2s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 89d2664. ± Comparison against base commit 80c4928.

♻️ This comment has been updated with latest results.

@nadime15
Copy link

nadime15 commented Feb 19, 2025

I just noticed that sstatus also has a UBE bit. It should be added to bitfield Sstatus including lower_mstatus() and lift_sstatus()

@jordancarlin jordancarlin linked an issue Feb 20, 2025 that may be closed by this pull request
@pmundkur
Copy link
Contributor

The readme can also be updated to mention this support.

Ok(v, m) }
(_, Some(v, m)) => {
let temp_var: bits('n * 8) = match (t, is_big_endian(t)) {
(Execute(), _) => (v),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not move the check for an execute access into is_big_endian? It already needs to get AccessType to determine effective privilege, and if it is an execute type it will always be little endian.

then print_mem("mem[" ^ to_str(t) ^ "," ^ BitStr(physaddr_bits(paddr)) ^ "] -> " ^ BitStr(v));
Ok(v, m) }
(_, Some(v, m)) => {
let temp_var: bits('n * 8) = match (t, is_big_endian(t)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use a more specific/significant name than temp_var. Especially as we move to include Sail code in the ISA manual, we want to make sure the meaning of everything is clear.

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.

Support for mstatus MBE, SBE
4 participants