Replace old virtual memory code with new (details follow) #365

rsnikhil · 2023-12-01T20:37:16Z

Old 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_common.sail riscv_pte.sail riscv_ptw.sail
riscv_vmem_rv32.sail riscv_vmem_rv64.sail
riscv_vmem_sv32.sail riscv_vmem_sv39.sail riscv_vmem_sv48.sail

Added 1 new file, where the main vmem code sits:
riscv_vmem.sail

Modified (to be compatible with new code, and to isolate TLB stuff
into one file, since TLBs are not part of RISC-V Architecture Spec)

riscv_vmem_tlb.sail

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

github-actions · 2023-12-01T21:03:05Z

Test Results

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

Results for commit 53a116e. ± Comparison against base commit d7a3d80.

♻️ This comment has been updated with latest results.

Alasdair

I've added a bunch of comments. Mostly smaller style related issues, I hope you don't mind the nitpicking!

For what I suggested in our meeting, I found there's a bug during the Sail->C path where Sail will try to create a function that can produce an undefined element of any custom datatype, to allow you to write let x : custom_datatype = undefined. That seems to be falling over for the numeric set types I wanted to use 😢

I'll fix that bug but in the mean time, there's an easy workaround where you can just add your own undefined_custom_datatype function and it'll use it. Since we don't create any undefined SV_Params it can just return some valid instance. I think it's probably still worth it because the Sail->C generation will be much more efficient it we use finite types rather than nat.

I know you mostly generalized the existing code, but maybe it would be good to roll in some fixing of naming conventions. I'm not sure why we have translateAddr rather than translate_addr. Alternatively we could leave that out of this PR for a follow-up, I don't have a strong opinion either way, other than I would like it to eventually be more consistent.

model/riscv_vmem.sail

model/riscv_vmem_tlb.sail

Makefile

model/riscv_vmem_tlb.sail

rsnikhil · 2023-12-04T19:41:26Z

My apologies. I've re-marked it 'unresolved'. I also don't know who is supposed to mark it 'resolved' (me? or the commenter?). [ I'm new to multi-person/multi-org projects on GitHub, and am not familiar with the flows and protocols. I was marking each comment resolved as I dealt with it in my local clone, re-testing at each step. I had not yet pushed it to my GitHub fork.] I've just pushed a new commit (225b9cf) with my changes, to my fork (https://github.com/rsnikhil/sail-riscv). This started a CI run on my fork, which reported success. In the PR in upstream repo (https://github.com/riscv/sail-riscv), it reports this commit, and reran a CI, which reports success. Nikhil

…

On Mon, Dec 4, 2023 at 1:44 PM Alexander Richardson < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In model/riscv_vmem.sail <#365 (comment)>: > + satp_ppn_size_b : nat, // 22 44 44 + satp_ppn_lsb_index : nat, // 0 0 0 + + // VA // Sv32 Sv39 Sv48 + va_size_b : nat, // 32 39 48 + vpn_size_b : nat, // 10 9 9 + + // PTE // Sv32 Sv39 Sv48 + levels : nat, // 2 3 4 + log_pte_size_B : nat, // 2 3 3 + pte_msbs_lsb_index : nat, // 32 54 54 + pte_msbs_size_b : nat, // 0 10 10 + pte_PPNs_lsb_index : nat, // 10 10 10 + pte_PPNs_size_b : nat, // 22 44 44 + pte_PPN_j_size_b : nat // 10 9 9 +} Could you please upload the new version before marking comments as resolved? — Reply to this email directly, view it on GitHub <#365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5QDSP7XIOYB5SDTQAFC7LYHYKZDAVCNFSM6AAAAABADL3UHSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRTGE2TCOJXHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

model/riscv_vmem.sail

Timmmm

Sail really needs module support!

model/riscv_vmem.sail

Timmmm · 2023-12-05T12:55:40Z

model/riscv_vmem.sail

+}
+
+// PRIVATE: extract LSBs (including permissions) of PTE
+function D_of_PTE(pte : bits(64)) -> bit = pte[7]


Probably makes sense to use a bitfield for this?

The original code had three separate bitfields for PTEs for Sv32, Sv39 and Sv48, much of the PTEs are different for each one. To unify these (since bitfields are not allowed to be polymorphic at the moment), I resorted to working at 64bits throughout and using selector functions, and using SV_Params to parameterize the differences.

Of course, the bottom 8 bits of PTEs are the same for Sv32, Sv39 and Sv48, and one could use a common bitfield for that part of the PTE, but I’m not sure it would clarify things much.

I think using a bitfield for the common bits would be slightly easier to read than these wrapper functions.

Ok, I’ll defer to the majority here and do the common 8 LSBs with a bitfield, although IMHO while bitfields are a nice construct, I don’t think it improves readability in this particular situation, especially since other fields in the PTE are not done this way.

Github won't let me actually resolve this for some reason but it's resolved.

Timmmm · 2023-12-05T12:59:59Z

model/riscv_vmem_tlb.sail

-/*  OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT                   */
-/*  OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF                   */
-/*  SUCH DAMAGE.                                                                         */
+/*  RISCV Sail Model									 */


Whitespace seems wrong here.

I don’t think I understand the display above. The above cited excerpt looks like this:

/* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT */ /* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF */ /* SUCH DAMAGE. */ /*=======================================================================================*/ // Although a TLB is not part of the RISC-V Architecture // specification, we model a simple TLB so that

The “RISCV Sail Model” line is on line 2, not after line 68.

The whitespace looks ok on my Emacs screen.

In VSCode, this whole header section initially looks messed up, because by default it has a “Spaces: 2” setting. On the bottom line, where it says “Spaces: 2”, this needs to be changed to “Indent using spaces” and setting the tab size to 8, to match the way Emacs displays it.

I think some spaces have been replaced with hard tabs.

PS: I highly recommend setting Render Whitespace to boundary in VSCode.

I think some spaces have been replaced with hard tabs.

Is that a problem? CODE_STYLE.md is silent on any policy regarding tabs vs. spaces.

PS: I highly recommend setting Render Whitespace to boundary in VSCode.

I rely on Emacs’ ’tabify’ and ‘untabify’ functions to do tabs/spaces properly for a selected region or buffer.

If Emacs and VSCode (and possibly vi) do it differently, I suggest not relying either one as a standard, as long as each tools is able to display it properly.

This still needs to be fixed.

Github won't let me actually resolve this for some reason but it's resolved.

Alasdair · 2023-12-05T13:20:02Z

Sail really needs module support!

I have one now, and I recently made a modularised sail-riscv to test it. I could open a PR for discussion.

…n PR riscv#365

model/riscv_vmem.sail

Timmmm · 2023-12-06T13:40:42Z

I have one now, and I recently made a modularised sail-riscv to test it. I could open a PR for discussion.

Great news - yes please!

Alasdair · 2023-12-12T13:59:59Z

It looks like the current code has tabs as well as spaces, we should make sure all the tabs are gone before we merge.

I thought we had a pre-commit hook to check for no tabs, but it looks like we don't.

rsnikhil · 2023-12-12T15:02:04Z

It looks like the current code has tabs as well as spaces, we should make sure all the tabs are gone before we merge.

I thought we had a pre-commit hook to check for no tabs, but it looks like we don't.

Yes it has tabs; I was unaware that no-tabs is the recommended style; CODE_STYLE.md is silent on this (perhaps someone should add this recommendation to CODE_STYLE.md?).

I actually applied “tabify()” in Emacs to convert spaces to tabs, because that is my normal practice in my own projects.

I will apply “untabify()” in Emacs to reverse that.

jrtc27 · 2023-12-12T15:04:47Z

CODE_STYLE.md clearly states a two space indent, because that was (and still is) the predominant style in the model.

rsnikhil · 2023-12-12T15:10:45Z

CODE_STYLE.md clearly states a two space indent, because that was (and still is) the predominant style in the model.

Yes, I read that, and am aware of that, but it does not rule out using tabs to reach the context indent before applying two more spaces. And there were tabs in other long white spaces (e.g., in the license text).

In any case: I’ve applied ‘untabify()’ to the wholef file to remove all tabs.

jrtc27 · 2023-12-12T15:12:48Z

CODE_STYLE.md clearly states a two space indent, because that was (and still is) the predominant style in the model.

Yes, I read that, and am aware of that, but it does not rule out using tabs to reach the context indent before applying two more spaces. And there were tabs in other long white spaces (e.g., in the license text).

In any case: I’ve applied ‘untabify()’ to the wholef file to remove all tabs.

Tabs are not spaces, and don’t even have a fixed column width. If you convert spaces to tabs then you don’t have a 2 space per block indent, you have N tabs and M spaces.

Alasdair · 2023-12-12T15:22:23Z

We have too much code that tries to align things in a pretty way to use tabs, as it'll just look broken in anyone else's editor if they don't have the same tab width.

The error message formatting code in Sail also just assumes a tab width of 4, so error messages will look off to anyone who has set their tab width differently.

rsnikhil · 2023-12-12T15:24:37Z

CODE_STYLE.md clearly states a two space indent, because that was (and still is) the predominant style in the model.

Yes, I read that, and am aware of that, but it does not rule out using tabs to reach the context indent before applying two more spaces. And there were tabs in other long white spaces (e.g., in the license text).
In any case: I’ve applied ‘untabify()’ to the wholef file to remove all tabs.

Tabs are not spaces, and don’t even have a fixed column width. If you convert spaces to tabs then you don’t have a 2 space per block indent, you have N tabs and M spaces.

Ok, that’s not how I (and possibly others) interpret the language. IMO, a phrase “don’t use tabs at all” would clarify it. It’s a moot point anyway, now, for the files in question, since I’ve converted everything to spaces.

rsnikhil · 2023-12-12T15:33:29Z

We have too much code that tries to align things in a pretty way to use tabs,
as it'll just look broken in anyone else's editor if they don't have the same tab width.

I agree, and I’m fine with a policy of “no-tabs”. I’m just asking that we state that explicitlyl in CODE_STYLE.md. For example, there could be wide spaces in comments (and there are wide spaces in the license text) which, if done with tabs, would also look wrong, even though they don’t have anything to do with code indentation policy.

Alasdair · 2023-12-12T15:37:30Z

I made a PR here #372

Timmmm

Some more review comments. Please can you fix the formatting? Then I will do some more reviewing.

Timmmm · 2023-12-12T12:18:08Z

model/riscv_vmem_tlb.sail

-/*  OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT                   */
-/*  OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF                   */
-/*  SUCH DAMAGE.                                                                         */
+/*  RISCV Sail Model									 */


This still needs to be fixed.

Timmmm · 2023-12-12T12:18:42Z

model/riscv_vmem_tlb.sail

+// specification, we model a simple TLB so that
+// (1) we can meaningfully test SFENCE.VMA which would be a no-op wihout a TLB;
+// (2) we can greatly speed up simulation speed (for Linux boot, can
+//     reduce elapsed time from 10s of minutes to few minutes.


Missing )!

Timmmm · 2023-12-12T12:58:15Z

model/riscv_vmem.sail

+
+// This two-line idiom constrains the value (2nd line) to be a singleton,
+// which helps in type-checking wherever it is used.
+type PAGESIZE_BITS : Int = 12


The rest of the code uses lowercase for these. Also I think it's proabbly unwise to use the same name for both of these.

The rest of the code uses lowercase for these. Also I think it's proabbly unwise to use the same name for both of these.

I’m not sure what you mean by “rest of the code uses lowercase for these”. grep shows no other use of PAGESIZE_BITS, upper or lower case, in any file other than my new files (riscv_vmem.sail, riscv_vmem_tlb.sail) where they are all uppercase. Are you perhaps referring to code in some other repo or branch (CHERI)?

I was following Jessica’s suggestion exactly: Replace old virtual memory code with new (details follow) #365 (comment) which has both the type and the value in upper case.

But, in any case, I take your point, and I will change the value-use to lowercase.

By "these" I meant global type definitions. If you search for : Int = in this repo or the CHERI one they are all lowercase. I think Jessica was just copying your style.

Though they do use exactly the same name for both definitions in the CHERI code, so maybe it is ok to just do this:

type pagesize_bits : Int = 12 let pagesize_bits = sizeof(pagesize_bits)

I’ve retained uppercase for the type def and changed the value def to lowercase (Haskell-like). Perhaps we should adopt a uniform convention across the whole repo.

Timmmm · 2023-12-12T12:58:54Z

model/riscv_vmem.sail

+       satp_ppn_lsb_index  = 0,
+
+       // VA
+       va_size_bits	   = 32,


Indentation of va_size_bits and levels is a bit wrong in all 4 of these.

I think this was just a tabs artefact. I’ve removed all tabs.

Timmmm · 2023-12-12T12:59:35Z

model/riscv_vmem.sail

+
+// PRIVATE: Extract full VPN field from VA
+function vpns_of_va(sv_params : SV_Params,
+		    va	      : bits(64)) -> bits(64) = {


Indentation. I think you just have a load of hard tabs that need to be replaced with spaces.

I’re removed all tabs, for spaces.

Timmmm · 2023-12-12T16:11:16Z

model/riscv_vmem.sail

+}
+
+// PRIVATE: Extract offset within page from VA
+function offset_of_va(va : bits(64)) -> bits(64) = {


function offset_of_va(va : bits(64)) -> bits(PAGESIZE_BITS) = va[PAGESIZE_BITS - 1 .. 0]

Ok (shrinking output type from bits(64) to bits[PAGESIZE_BITS] needed a compensating zero_extend() in the two places where it was invoked).

Timmmm · 2023-12-12T16:12:01Z

model/riscv_vmem.sail

+}
+
+// PRIVATE: check if a PTE is a pointer to next level (non-leaf)
+function pte_is_ptr(pte_lsbs : PTE_LSBs) -> bool = (  pte_lsbs.X() == 0b0)


Extra spaces

Timmmm · 2023-12-12T16:12:54Z

model/riscv_vmem.sail

+  let pte_R = pte_lsbs.R();
+  let pte_W = pte_lsbs.W();
+  let pte_X = pte_lsbs.X();
+  let b : bool =


b is not a very descriptive name. How about success?

Timmmm · 2023-12-12T16:15:08Z

model/riscv_vmem.sail

+// walks during address translation; it typically works in conjunction
+// with any customization to check_PTE_permission().
+
+// PRIVATE


Btw I wonder if we should adopt the "leading underscore = private" convention, given that Sail doesn't support public/private natively yet.

Maybe after modules are added.

(No changes needed here; just a thought).

Let’s wait for modules, which I believe will be available soon.

Timmmm · 2023-12-12T16:17:44Z

model/riscv_vmem.sail

+  let vpn_j	 = vpn_j_of_va(sv_params, va, level);
+  let pte_offset = vpn_j << sv_params.log_pte_size_bytes;
+  let pte_addr	 = pt_base + pte_offset;
+  let pte_phys_addr : xlenbits = pte_addr[(sizeof(xlen) - 1) .. 0];


Is this right? Physical addresses are 34 bits with SV32.

Maybe a bug in mem_read_priv() I guess. Worth adding a TODO comment at least.

Yes, Sv32 phys mem addrs are 34 bits, not 32 bits. But it is used in 'mem_read_priv()' [riscv_mem.sail, L180], where the address argument is declared as 'xlenbits', so until mem_read_priv() is changed, we have to use 32 bits.

I don't know if it's straightforward to fix 'mem_read_priv()'. As I understand it, in RV32, phys mem addrs are 32(34) bits when Sv32 is not (is) supported. Even if Sv32 is supported, what happens when we run with M privilege (are phys addrs 32 bits? Or extended to 34 bits)?

I've mentioned this several times in our meetings (perhaps I should add an "issue" so it doesn't slip thru the cracks), and I'll also add a local TODO comment here in the vmem code.

Physical addresses at 34 bits if Sv32 is implemented, but Mmode cannot access beyond 32 bits
unless it sets (mstatus.MPRV==1) && (mstatus.MPP !=mmode) && (satp.mode!=0).
If those are set correctly, then it will use VM translation and use all 34 bits.
Otherwise, the upper 2 bits can't be used in Mmode, and they're set to zero.

arichardson · 2024-01-02T21:27:27Z

model/riscv_vmem.sail

@@ -120,6 +120,9 @@ struct SV_Params = {
  pte_PPN_j_size_bits : {10,  9}         //   10     9     9
 }

+// Current level during a page-table walk (0 to SV_Params.levels - 1)
+type Level = range(0,3)    // range(0,4) when add Sv57 (TODO)


Level is quite a generic name. Until there is a real way of declaring this file-local in sail, I think we have to use something like PTELevel here.

Ok, changed name to PTW_Level

Timmmm

Hey, sorry for the review delay - I missed that you replied.

Generally this looks good to me. Review summary:

One bit definitely looks like a bug (in update_PTE_Bits).
Lots of unnecessary explicit type annotations. I think it's better to leave those out in almost all cases.
Lots of unnecessary brackets around expressions. Sometimes it clarifies things, but I think if you have let foo = (...); then it just adds noise.
Probably the biggest thing that needs to change is to move functions back into riscv_pte.sail/riscv_ptw.sail so that CHERI can override functions without having to completely copy & paste the page table walker.
Some other misc stuff.

For CHERI I don't think we need to maintain exactly the same function signatures - some small amount of updating work is acceptable there I think. But we should maintain roughly the same code structure so it's at least feasible. And yeah it is kind of annoying that there's no clear indication exactly what the "public overidable" interfaces are, but I guess we'll just have to put up with that for now.

Timmmm · 2024-01-09T11:36:05Z

model/riscv_vmem_tlb.sail

+  }
+
+// PRIVATE
+function add_to_TLB(asid          : bits(16),


Could be worth a typedef for this? type asidbits = bits(16)

Timmmm · 2024-01-09T11:36:30Z

model/riscv_vmem_tlb.sail

+                    global        : bool,
+                    levelBitSize  : nat,
+                    PAGESIZE_BITS : nat)     -> unit = {
+  let shift      : nat       = PAGESIZE_BITS + (level * levelBitSize);


Don't think you need the : nat here.

Ok. Removed.

Timmmm · 2024-01-09T11:36:51Z

model/riscv_vmem_tlb.sail

+  let shift      : nat       = PAGESIZE_BITS + (level * levelBitSize);
+  assert(shift <= 64);
+  let vAddrMask  : bits(64)  = zero_extend(ones(shift));
+  let vMatchMask : bits(64)  = ~ (vAddrMask);


Can remove : bits(64).

In this case I think it serves as useful documentation, since 'zero_extend (ones (shift))' is polymorphic and so the result width may not be obvious.

Timmmm · 2024-01-09T11:44:28Z

model/riscv_vmem.sail

+                             ReadWrite(_, _) => true
+                           });
+  // Update 'accessed'-bit?
+  let update_a : bool = pte_lsbs.A() == 0b0;


Can remove : bool.

Ok. Removed. But I’ve added parens around the RHS because I’m nervous when ‘=‘ and ‘==‘ are on the same line, especially for someone coming from a C background.

Timmmm · 2024-01-09T11:45:03Z

model/riscv_vmem.sail

+  let update_a : bool = pte_lsbs.A() == 0b0;
+
+  if update_d | update_a then {
+    let pte_lsbs1 = update_A (pte_lsbs, 0b1);


[pte_lsbs with A = 0b1] (apparently the update_A is old syntax and I definitely prefer the new syntax!)

Ok, fixed. I was unaware of the new syntax.

Timmmm · 2024-01-09T12:37:39Z

model/riscv_vmem.sail

+              n_ent : TLB_Entry = ent;
+              n_ent.pte = pte';
+              write_TLB(tlb_index, n_ent);
+              let pte_phys_addr : xlenbits = ent.pteAddr[(sizeof(xlen) - 1) .. 0];


Can delete the : xlenbits.

Timmmm · 2024-01-09T12:38:25Z

model/riscv_vmem.sail

+            TR_Failure(PTW_PTE_Update(), ext_ptw)
+          else {
+            // Writeback the PTE (which has new A/D bits)
+            let pte_phys_addr : xlenbits = pteAddr[(sizeof(xlen) - 1) .. 0];


Delete : xlenbits.

Timmmm · 2024-01-09T12:38:54Z

model/riscv_vmem.sail

+  let vAddr_64b : bits(64) = zero_extend(vAddr);
+  // Effective privilege takes into account mstatus.PRV, mstatus.MPP
+  // See riscv_sys_regs.sail for effectivePrivilege() and cur_privilege
+  let effPriv   : Privilege = effectivePrivilege(ac, mstatus, cur_privilege);


Can delete : Privilege and : SATPMode and : ext_ptw

Here I think the annotations serve a useful documentation purpose. The RHS functions are non-local and the type of their results may not be obvious. Personally I prefer more rather than less type annotations.

Timmmm · 2024-01-09T12:40:19Z

model/riscv_vmem.sail

+  if not(valid_va) then
+    TR_Failure(translationException(ac, PTW_Invalid_Addr()), ext_ptw)
+  else {
+    let mxr    : bool     = mstatus.MXR() == 0b1;


Delete all these type annotations.

Ok (partial). I’ve deleted them for the obvious boolean exprs, but not for the others, where I think they serve as useful documentation.

Timmmm · 2024-01-09T12:41:29Z

model/riscv_vmem.sail

+  let effPriv   : Privilege = effectivePrivilege(ac, mstatus, cur_privilege);
+  let mode      : SATPMode = translationMode(effPriv);
+  // PTW extensions (non-standard): initialize the PTW extension state
+  let ext_ptw   : ext_ptw = init_ext_ptw;


Why make this copy? Just use init_ext_ptw directly.

riscv#365 (review)

Timmmm · 2024-02-02T12:49:22Z

Just wanted to say I haven't forgotten about this - I'm just working on testing it.

Alasdair · 2024-02-19T01:01:19Z

If you don't want to continue, would it be possible for me to take over this PR? At the very least I would like this on my own fork, even if nothing can be merged here

rsnikhil · 2024-02-19T01:31:38Z

If you don't want to continue, would it be possible for me to take over this PR? At the very least I would like this on my own fork, even if nothing can be merged here

Hi Alasdair, no I will see it through. This PR (#365) got marked as 'closed' because of my ham-handedness with GitHub/git. I updated my fork of sail-riscv on GitHub to sync up with the latest commits, then did 'git pull - rebase' to my clone on my laptop, and (a) it closed the PR and (b) threw out all my changes. Fortunately I had a backup of all my changes. I have re-incorporated my changes and re-tested, and it looks ok. I'll create a new PR shortly. Apologies for the confusion.

Alasdair · 2024-02-19T01:47:24Z

Ah, no worries then. I misinterpreted you closing the PR.

That said I remember you said you would be busy at some point soon, so if you do need someone to help drive this forwards please don't hesitate to ask me.

rsnikhil · 2024-02-19T03:30:17Z

Ah, no worries then. I misinterpreted you closing the PR.

That said I remember you said you would be busy at some point soon, so if you do need someone to help drive this forwards please don't hesitate to ask me.

I have just now created PR #408 for this (it is running CI tests, which already passed in my fork).

I also separated out code into riscv_vmem_pte.sail and riscv_vmem_ptw.sail per @Timmmm ’s suggestion for convenience for CHERI project which, as I understand it, substitute these files with CHERI-specific files.

It’s been ready for merge for some time now (all PR comments addressed). @Timmmm said he’d run it through some additional tests that are not part of the CI tests.

Timmmm · 2024-02-22T12:20:28Z

I haven't forgotten! Give me another week or so...

Alasdair reviewed Dec 1, 2023

View reviewed changes

arichardson reviewed Dec 1, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Alasdair reviewed Dec 2, 2023

View reviewed changes

model/riscv_vmem_tlb.sail Outdated Show resolved Hide resolved

arichardson reviewed Dec 4, 2023

View reviewed changes

model/riscv_vmem.sail Outdated Show resolved Hide resolved

Timmmm requested changes Dec 5, 2023

View reviewed changes

rsnikhil added a commit to rsnikhil/sail-riscv that referenced this pull request Dec 5, 2023

In riscv_vmem{_tlb}.sail, more style improvements based on comments i…

ded1c57

…n PR riscv#365

defermelowie reviewed Dec 6, 2023

View reviewed changes

model/riscv_vmem.sail Outdated Show resolved Hide resolved

rsnikhil added a commit to rsnikhil/sail-riscv that referenced this pull request Dec 6, 2023

Further tweaks based on recent comments on PR riscv#365

98351d2

Timmmm requested changes Dec 12, 2023

View reviewed changes

arichardson reviewed Jan 2, 2024

View reviewed changes

Timmmm requested changes Jan 9, 2024

View reviewed changes

rsnikhil added a commit to rsnikhil/sail-riscv that referenced this pull request Jan 14, 2024

Small local modifications to address PR review comments:

53a116e

riscv#365 (review)

rsnikhil closed this Feb 19, 2024

rsnikhil force-pushed the master branch from 53a116e to 9602e3a Compare February 19, 2024 00:03

rsnikhil mentioned this pull request Feb 19, 2024

Replace old virtual memory code with new (details follow) #408

Merged

Replace old virtual memory code with new (details follow) #365

Replace old virtual memory code with new (details follow) #365

Conversation

rsnikhil commented Dec 1, 2023

github-actions bot commented Dec 1, 2023 • edited Loading

Test Results

Alasdair left a comment

Choose a reason for hiding this comment

rsnikhil commented Dec 4, 2023 via email

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alasdair commented Dec 5, 2023

Timmmm commented Dec 6, 2023

Alasdair commented Dec 12, 2023

rsnikhil commented Dec 12, 2023

jrtc27 commented Dec 12, 2023

rsnikhil commented Dec 12, 2023

jrtc27 commented Dec 12, 2023

Alasdair commented Dec 12, 2023

rsnikhil commented Dec 12, 2023

rsnikhil commented Dec 12, 2023

Alasdair commented Dec 12, 2023

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsnikhil Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 •

edited

Loading

rsnikhil Jan 14, 2024 •

edited

Loading