Svadu support #260

adlr · 2023-05-10T21:42:26Z

Draft change for Svadu support. I appreciate any feedback on this change. Thank you!

No tests were broken in the test/run_tests.sh suite.

Alasdair · 2023-05-10T21:49:54Z

I left a couple of comments but mostly looks good to me 👍

jrtc27 · 2023-05-10T21:46:46Z

model/riscv_sys_regs.sail

+  if high == false & sizeof(xlen) == 32 then
+    c
+  else {
+    // If 32bit, then high is true


Comments are C-style

Changed to C++ style

jrtc27 · 2023-05-10T21:48:30Z

model/riscv_sys_regs.sail

+register menvcfg : MEnvCfg
+
+function legalize_menvcfg(c : MEnvCfg, high : bool, value : xlenbits) -> MEnvCfg = {
+  if high == false & sizeof(xlen) == 32 then


did not(high) as requested

jrtc27 · 2023-05-10T21:49:14Z

model/riscv_sys_regs.sail

+      32 => value @ 0x0000_0000
+    };
+    let c = update_HADE(c, [full[61]]);
+    c


I feel like there must be a better way to write all this... is this the first *h CSR we've implemented that's not just a stub 0?

I can't say I'm anywhere close to a SAIL expert. I'm all ears on suggestions here. Thank you!

jrtc27 · 2023-05-10T21:49:52Z

model/riscv_vmem_sv32.sail

@@ -214,6 +210,9 @@ function translate32(asid, ptb, vAddr, ac, priv, mxr, do_sum, level, ext_ptw) =
                  MemException(e) => internal_error(__FILE__, __LINE__, "invalid physical address in TLB")
                };
                TR_Address(ent.pAddr | EXTZ(vAddr & ent.vAddrMask), ext_ptw)
+              } else {


Inverting these is a little gratuitous, though perhaps it makes more sense

Changed back the if/then ordering

jrtc27 · 2023-05-10T21:50:10Z

model/riscv_vmem_tlb.sail

+
+function should_update_PTE_bits() -> bool = {
+  plat_enable_dirty_update() | (plat_enable_svadu() & menvcfg.HADE() == 0b1)
+}


Missing newline

Added newline back

jrtc27 · 2023-05-10T21:50:55Z

I left a couple of comments but mostly looks good to me 👍

Your comments appear to be missing

arichardson · 2023-05-10T21:51:16Z

model/riscv_vmem_sv48.sail

-            /* pte needs dirty/accessed update but that is not enabled */
-            TR_Failure(PTW_PTE_Update(), ext_ptw)
-          } else {
+          if should_update_PTE_bits() then {


Is there any reason to swap branch order here instead of using not(should_update_PTE_bits())? Not objecting to this just wondering why.

I think it's slightly nicer to avoid the not

I changed it back, figuring that without a clear consensus, I'd minimize the diff.

Alasdair · 2023-05-10T21:48:44Z

model/riscv_sys_regs.sail

+      32 => value @ 0x0000_0000
+    };
+    let c = update_HADE(c, [full[61]]);
+    c


I think this let c = ... in c could just be update_HADE(c, [full[61])

I tried this, but got an error. So, leaving the let c = in

Please show the error; it should work just fine

You need to remove the semicolon at the end because you're returning the value from update_HADE.

Did you try:

update_HADE(c, [full[61]]); c

Because that would result in the error you posted, what I meant was:

update_HADE(c, [full[61]])

As the last expression in the block

That did it! Seems obvious now, sorry. Will post a new patch set later, just wanted to quickly share that this did work as you suggested. Thanks!

Alasdair · 2023-05-10T21:49:13Z

model/riscv_sys_regs.sail

+    c
+  else {
+    // If 32bit, then high is true
+    let full : bits(64) = match (sizeof(xlen)) {


I think the brackets around sizeof are not-needed

github-actions · 2023-05-10T22:19:05Z

Unit Test Results

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

Results for commit 10379c8. ± Comparison against base commit c33a6f1.

♻️ This comment has been updated with latest results.

Alasdair · 2023-05-10T22:21:38Z

I'm not very familiar with the RISC-V address translation scheme, but looking a bit deeper at the functionality, I think you also need to update the lookup case for a TLB miss (the None() branch with the if not(plat_enable_dirty_update()) at line 232 in riscv_vmem_sv32.sail (and similarly for the other sizes). It looks like the 'TLB' is essentially an emulation optimization (which every architectural model ends up with for reasonable performance), so the real architectural behavior is really specified by walk32.

Obviously not a task for yourself, but I don't think we need multiple files for the differently sized translations - unless there is significantly differing behaviour I think it could be a single size-polymorphic function. I'm also not sure we are implementing step 7 of the translation process as written in the priv spec, at least I'm not seeing how that is done in the code? I'm wondering about this because the Svadu extension seems to modify this check slightly to avoid a page fault, but I don't see where that is being done.

Edit:

After some more reading it seems like the A/D bits update is being done in update_PTE_bits in riscv_pte.sail. I am wondering if that needs to be updated (specifically the check on line 147) for this paragraph in the Svadu document?

The translation of virtual addresses (or guest physical addresses) to physical (or guest physical)
addresses is accomplished with the same algorithm as specified in the Supervisor-Level ISA
extension (section 4.3.2) and as modified by the hypervisor extension (section 8.5.1), except that
step 7 of the translation process, instead of causing a page-fault exception due to A and/or D bits
being 0 when required to be 1, continues as follows:

adlr · 2023-05-23T19:46:09Z

I'm not very familiar with the RISC-V address translation scheme, but looking a bit deeper at the functionality, I think you also need to update the lookup case for a TLB miss (the None() branch with the if not(plat_enable_dirty_update()) at line 232 in riscv_vmem_sv32.sail (and similarly for the other sizes). It looks like the 'TLB' is essentially an emulation optimization (which every architectural model ends up with for reasonable performance), so the real architectural behavior is really specified by walk32.

Obviously not a task for yourself, but I don't think we need multiple files for the differently sized translations - unless there is significantly differing behaviour I think it could be a single size-polymorphic function. I'm also not sure we are implementing step 7 of the translation process as written in the priv spec, at least I'm not seeing how that is done in the code? I'm wondering about this because the Svadu extension seems to modify this check slightly to avoid a page fault, but I don't see where that is being done.

Edit:

After some more reading it seems like the A/D bits update is being done in update_PTE_bits in riscv_pte.sail. I am wondering if that needs to be updated (specifically the check on line 147) for this paragraph in the Svadu document?

The translation of virtual addresses (or guest physical addresses) to physical (or guest physical)
addresses is accomplished with the same algorithm as specified in the Supervisor-Level ISA
extension (section 4.3.2) and as modified by the hypervisor extension (section 8.5.1), except that
step 7 of the translation process, instead of causing a page-fault exception due to A and/or D bits
being 0 when required to be 1, continues as follows:

I think we've adjusted the code to handle the additional cases Alasdair raised.

Thanks very much for the reviews!

arichardson · 2023-05-23T21:55:24Z

model/riscv_vmem_tlb.sail

@@ -120,3 +120,7 @@ function flush_TLB_Entry(e, asid, addr) = {
                           & not(e.global))
  }
 }
+
+function should_update_PTE_bits() -> bool = {
+  plat_enable_dirty_update() | (plat_enable_svadu() & menvcfg.HADE() == 0b1)


I wonder if the logic should be changed such that enable-dirty-update implicitly enables svadu and sets the initial value of menvcfg.HADE to 1? In that case this function could be simplified to plat_enable_svadu() & menvcfg.HADE() == 0b1. I believe the --enable-dirty-update flag was only added because there was no Svadu at the time and some operating systems expect the PTE updates.

No, because then you can't configure the model to be an implementation that does A/D updates but doesn't have Svadu, a privileged spec option which was ratified years ago.

That is true, but looking at this PR there are no observable behaviour changes for plat_enable_svadu()/not(plat_enable_svadu()) when plat_enable_dirty_update() is also true? Does this mean we need to also guard write access to menvcfg.HADE by plat_enable_svadu()? Those older implementations should have that bit hardwired to zero (well most likely not have the register at all)?

Yes, HADE shouldn't exist when Svadu isn't enabled, and needs to be legalised as 0

That makes sense and I agree we should have two separate options to allow matching those implementations.

arichardson · 2023-05-23T22:09:02Z

model/riscv_sys_regs.sail

+      64 => value,
+      32 => value @ 0x0000_0000
+    };
+    let c = update_HADE(c, [full[61]]);


Looking at the Svadu spec I see: When menvcfg.HADE is zero, the implementation behaves as though Svadu were not implemented. If Svadu is not implemented, menvcfg.HADE is read-only zero. so I believe this needs to be guarded by plat_enable_svadu()

Thanks! I needed to move plat_enable_svadu() out of riscv_platform.sail and added a note similar to the one by get_16_random_bits explaining it, but I made this change

arichardson · 2023-06-02T15:34:44Z

model/riscv_sys_regs.sail

+      64 => value,
+      32 => value @ 0x0000_0000
+    };
+    if not(plat_enable_svadu())


Isn't this inverted? We should be able to toggle HADE if svadu is enabled.

Good catch. I inverted this and cleaned up the legalize_menvcfg function. Thank you!

arichardson · 2023-06-05T20:34:00Z

model/riscv_sys_regs.sail

+function legalize_menvcfg_high(c : MEnvCfg, value : bits(32)) -> MEnvCfg = {
+  /* Subtract 32 from bit offsets as we only have high half here */
+  if plat_enable_svadu()
+  then update_HADE(c, [value[61 - 32]])
+  else c
+}


Suggested change

function legalize_menvcfg_high(c : MEnvCfg, value : bits(32)) -> MEnvCfg = {

/* Subtract 32 from bit offsets as we only have high half here */

if plat_enable_svadu()

then update_HADE(c, [value[61 - 32]])

else c

}

function legalize_menvcfg_high(c : MEnvCfg, value : bits(32)) -> MEnvCfg = {

let newval = Mk_MEnvCfg(value @ to_bits(32, 0));

if plat_enable_svadu()

then update_HADE(c, newval.HADE())

else c

}

How about the following to avoid the subtraction (which will become awkward once we add more fields here.

Thanks! Made the edits and credited you

ved-rivos · 2023-08-12T17:34:03Z

@adlr - the extension name was changed from Svadu to Svhad. The fields in CSRs continue to be HADE. Just extension name changed.

Co-authored-by: Aaron Durbin <[email protected]> Co-authored-by: Alex Richardson <[email protected]>

jrtc27 reviewed May 10, 2023

View reviewed changes

arichardson reviewed May 10, 2023

View reviewed changes

Alasdair reviewed May 10, 2023

View reviewed changes

adlr force-pushed the svadu branch from 12e0d62 to e9d7e7f Compare May 23, 2023 19:42

arichardson reviewed May 23, 2023

View reviewed changes

adlr force-pushed the svadu branch from e9d7e7f to 10379c8 Compare May 24, 2023 23:04

ptomsich force-pushed the master branch from 917e6c0 to f2d8e1b Compare May 29, 2023 19:07

arichardson reviewed Jun 2, 2023

View reviewed changes

adlr force-pushed the svadu branch from 10379c8 to 647d418 Compare June 3, 2023 22:10

arichardson reviewed Jun 5, 2023

View reviewed changes

adlr force-pushed the svadu branch from 647d418 to 5e53575 Compare June 6, 2023 18:35

adlr force-pushed the svadu branch from 5e53575 to 181da73 Compare July 6, 2023 02:36

ved-rivos mentioned this pull request Jul 30, 2023

Add ratified Svadu extension riscv-non-isa/riscv-arch-test#371

Merged

10 tasks

adlr force-pushed the svadu branch from 181da73 to fc83c7a Compare August 15, 2023 17:08

Svadu support

0834634

Co-authored-by: Aaron Durbin <[email protected]> Co-authored-by: Alex Richardson <[email protected]>

adlr force-pushed the svadu branch from fc83c7a to 0834634 Compare August 15, 2023 17:30

arichardson mentioned this pull request Oct 26, 2023

Add Svadu support #327

Closed

Timmmm added the extension Adds support for a RISC-V extension label Jan 6, 2025

Svadu support #260

Are you sure you want to change the base?

Svadu support #260

Conversation

adlr commented May 10, 2023

Alasdair commented May 10, 2023

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

jrtc27 commented May 10, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 10, 2023 • edited Loading

Unit Test Results

Alasdair commented May 10, 2023 • edited Loading

adlr commented May 23, 2023

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

ved-rivos commented Aug 12, 2023

jrtc27 commented May 10, 2023 •

edited

Loading

github-actions bot commented May 10, 2023 •

edited

Loading

Alasdair commented May 10, 2023 •

edited

Loading