Implementing callbacks in sail-riscv for state-changing events #494

kseniadobrovolskaya · 2024-06-11T11:57:24Z

I have an initial implementation for the suggestion described in #449 . However I still have some questions related to the implementation details and would like to get some advise (if possible).

I have provided a default implementation of notification callbacks for the C language. I use "weak" attributes, so users can override these default implementations. However, it is also necessary to provide this implementation for the Ocaml backend, where as I understand these attributes do not exist. Could someone advise how to resolve this?
In memory callback I'd like to set the changed bytes. But as I understood these bytes are stored in the internal type of sail lbits. Can you tell how I can convert this type to a ordinary type (e.g. const char *) so that users can provide implementation of callbacks without relying on lbits?

Timmmm

This looks along the lines I was thinking, though I think we will still need to have RVFI support in the C emulator, so you still need the flags. It will just need to be implemented via the callbacks.

I probably wouldn't use __attribute__((weak)). If anyone is modifying the callbacks they'll have their own fork of the emulator anyway and can just delete the default ones. That's what we (Codasip) will do.

I'll get you some code to read lbits.

model/riscv_insts_vext_vset.sail

Timmmm · 2024-06-11T13:39:38Z

It's something like this in C++ (adapted from our code, so this exact code isn't tested).

std::vector<uint8_t> readLbits(lbits val) {
    std::vector<uint8_t> data(val.len);

    // We should really double check the length of `val.bits` to ensure it
    // isn't actually greated than `val.len`, otherwise we are at risk of
    // a buffer overflow.
    // I think you can maybe use (mpz_sizeinbase(val.bits, 2) + 7) / 8.

    // Export the lbits. Note this doesn't pad with zeros but that's ok
    // because data is prefilled with zeros.
    mpz_export(
        // Destination buffer.
        data.data(),
        // Where to write the output size (we ignore this).
        nullptr,
        // Endianness of words (-1 = little).
        -1,
        // Size of words in bytes.
        1,
        // Endianness *within* words (0 = native). Doesn't matter in this case.
        0,
        // Number of "nail" bits (upper bits to clear in each word).
        0,
        // The GMP value. Note the sign is ignored but it should always be positive.
        val.bits);

    return data;
}

In crappy C you will have to deal with malloc manually rather than using vector.

arichardson · 2024-06-11T20:38:33Z

model/riscv_insts_vext_mem.sail

@@ -154,6 +154,7 @@ function process_vlsegff (nf, vm, vd, load_width_bytes, rs1, EMUL_pow, num_elem)
              if i == 0 then { ext_handle_data_check_error(e); return RETIRE_FAIL }
              else {
                vl = to_bits(sizeof(xlen), i);
+                if rv_enable_callbacks then csr_update_callback("vl", vl);


Having to add these manually is rather error prone and makes the code less readable. Can we do some fancy sail overloading tricks to automatically call these functions for CSR writes (e.g. something like CSR(vl) = ... ?

Various CSRs have a set_foo() function. We could put it in there. Though that doesn't significantly reduce the risk because people can always forget to use the setter.

I ran into a similar issue recently - trying to memoize/cache a value. Difficult to guarantee that all the code will use setter that updates the cache without some support for private.

IMO there are few enough of these that we can probably ignore this issue for the moment, though I agree it's not ideal.

Perhaps it would be better to get Sail to generate the callbacks automatically?

Yeah we discussed this before. Might be nice, but I'm not 100% sure it would do exactly what you want, e.g. you probably want distinct callbacks for writes to sstatus and mstatus, but under the hood they write to the same register. Also there are very few places this callback is needed so the manual burden is not that high, and I suspect Alasdair has enough on his plate already!

I added the ext_notification_write_CSR (and ext_notification_read_CSR) function. It helps to simplify writing to a CSR with a call to a callback.

Timmmm · 2024-06-11T21:52:54Z

mpz_sizeinbase(val.bits, 2) + 7) / 8

Actually reading this back, I wonder if mpz_sizeinbase(val.bits, 256) would work (and not be really slow).

kseniadobrovolskaya · 2024-06-13T14:47:00Z

I would like to point out that the callback functions are annotated as pure. But in reality we have no way to guarantee that users will not actually change the state of the model in these functions. Should we really keep this attribute or should we remove it so that we don't fool anyone?

Timmmm · 2024-06-13T17:56:17Z

I would like to point out that the callback functions are annotated as pure. But in reality we have no way to guarantee that users will not actually change the state of the model in these functions. Should we really keep this attribute or should we remove it so that we don't fool anyone?

I would say if someone edits the code and changes the callback to be impure then it's up to them to also change the Sail code to mark them as impure.

arichardson

Could you keep the RVFI-DII code around? Alternatively, a good demonstrator of the callbacks would be to implement the RVFI-DII socket interface using those callbacks?

Timmmm · 2024-06-13T21:34:44Z

Yeah to be clear I don't think these callbacks should be user defined. We should have hard-coded callbacks that support RVFI, logging, etc.

kseniadobrovolskaya · 2024-07-16T12:40:15Z

@arichardson, @Timmmm, what do you think of the current version?

Timmmm

Yeah this is pretty much exactly what I was thinking. I put some minor comments but I it looks like the right direction to me.

Timmmm · 2024-07-22T14:50:51Z

c_emulator/riscv_default_callbacks.c

+}
+int xreg_update_callback(unsigned reg, uint64_t value) { }
+int freg_update_callback(unsigned reg, uint64_t value) { }
+int csr_update_callback(const char *reg_name, uint64_t value) { }


Better to use the CSR index than the name. If this function wants to access the name it can call zcsr_name_map_forwards() or whatever.

Ok, now here's the argument is unsigned reg, and in the sail -- csreg.

Timmmm · 2024-07-22T14:53:56Z

model/riscv_insts_vext_mem.sail

@@ -153,25 +153,22 @@ function process_vlsegff (nf, vm, vd, load_width_bytes, rs1, EMUL_pow, num_elem)
            Ext_DataAddr_Error(e)  => {
              if i == 0 then { ext_handle_data_check_error(e); return RETIRE_FAIL }
              else {
-                vl = to_bits(sizeof(xlen), i);
-                print_reg("CSR vl <- " ^ BitStr(vl));
+                ext_notification_write_CSR(csr_name_map("vl"), to_bits(sizeof(xlen), i));


Not 100% sure about this change. For now I would probably just replace the print_regs with calls to csr_update_callback. We can do this in a future PR.

Done. Deleted ext_notification_write_CSR and replaced print_regs with calls to csr_update_callback.

Timmmm · 2024-07-22T14:54:43Z

ocaml_emulator/platform.ml

+let pc_update_callback value = ()
+let xreg_update_callback (reg, value) = ()
+let freg_update_callback (reg, value) = ()
+let csr_update_callback (reg_name, value) = ()


Minor naming but I'd replace update with write here. Everywhere else talks about writing registers and memory, not "updating" them.

Timmmm · 2024-07-22T14:58:03Z

model/riscv_types.sail

+$ifdef RVFI_DII
+register rv_enable_callbacks : bool = true
+$else
+register rv_enable_callbacks : bool = false


I think you can probably omit this and call them unconditionally. I doubt it will make any noticeable difference to performance and it will simplify the code.

Ok, I call them here unconditionally and check rv_enable_callbacks in the callback functions.

Timmmm · 2024-07-22T15:03:06Z

model/riscv_sys_control.sail

@@ -340,8 +340,7 @@ function trap_handler(del_priv : Privilege, intr : bool, c : exc_code, pc : xlen



There's a load of implicit CSR writes here that need to be tracked. We do it like this:

function track_trap(p : Privilege, intr : bool, c : exc_code) -> unit = { track_writeCSR(csr_name_map("mstatus")); match (p) { Machine => { track_writeCSR(csr_name_map("mcause")); track_writeCSR(csr_name_map("mtval")); track_writeCSR(csr_name_map("mepc")); }, Supervisor => { track_writeCSR(csr_name_map("scause")); track_writeCSR(csr_name_map("stval")); track_writeCSR(csr_name_map("sepc")); }, User => { track_writeCSR(csr_name_map("ucause")); track_writeCSR(csr_name_map("utval")); track_writeCSR(csr_name_map("uepc")); }, }; }

Timmmm · 2024-07-22T15:06:29Z

model/riscv_mem.sail

+    if rv_enable_callbacks then
+      match result {
+        MemValue(_) => mem_update_callback(paddr, width, value, /* is_exception */ false),
+        MemException(_) => mem_update_callback(paddr, width, value, /* is_exception */ true)


I don't know if it's necessarily better, but we only call out mem_read_callback() function on success MemValue(), and then we have another callback for traps in trap_handler(), where the print_reg()s are.

Maybe keep this (because you can easily ignore the exception case) but also add the trap callback.

You might want to add the exception code to the callback to make logging a bit easier:

match result { MemValue(_) => mem_write_callback(paddr, width, value), MemException(e) => mem_exception_callback(paddr, width, num_of_ExceptionType(e)),

Ok, added mem_exception_callback. But the only thing is, I call it without width since num_of_ExceptionType has type {'n, (0 <= 'n < xlen). int('n)} and width has type {'n, (0 < 'n <= max_mem_access). int('n)}. To simplify the function declaration in the sail. Especially in case of write failure only the memory address is logged and the width is not needed.

jrtc27 · 2024-07-22T15:55:55Z

model/riscv_fdext_regs.sail

@@ -157,6 +157,7 @@ function rF r = {
 val wF : forall 'n, 0 <= 'n < 32. (regno('n), flenbits) -> unit
 function wF (r, in_v) = {
  assert(sys_enable_fdext());
+  if rv_enable_callbacks then freg_update_callback(regno_to_regidx(r), in_v);


IMO a better design is to push the enable check down from the caller rather than replicate it everywhere

Ok, put rv_enable_callback into the riscv_sim.c and checking it in the callbacks.

arichardson

Overall I think this approach looks good, but I'd push all the rv_enable_callbacks into the callee to simplify the code. Since the variable is not a compile-time constant it shouldn't matter whether it's checked in the C code or sail code. It might be easier to keep this as an internal flag to the C/Ocaml emulators.

arichardson · 2024-07-26T22:18:03Z

model/riscv_fdext_regs.sail

-      /* TODO: will only print bits; should we print in floating point format? */
-      print_reg("f" ^ dec_str(r) ^ " <- " ^ FRegStr(v));
+
+  freg_write_callback(regno_to_regidx(r), in_v);


The default callback needs to print the register otherwise this removes tracing capabilities from the model.

Added an implementation of the callbacks for trace printing to the riscv_types.sail, riscv_csr_begin.sail, riscv_vreg_type.sail and call them from the default callbacks in the riscv_default_callbacks.c.

arichardson

Not reviewed this in detail, but overall the new approach looks sensible to me.

arichardson · 2024-10-10T23:09:34Z

model/riscv_types.sail

+val mem_write_callback_default : forall 'n, 0 < 'n <= max_mem_access . (/* addr */ xlenbits, /* width */ int('n), /* value */ bits(8 * 'n)) -> unit
+function mem_write_callback_default (addr, width, value) = {
+  print_mem("mem[" ^ BitStr(addr) ^ "] <- " ^ BitStr(value))
+}
+
+val mem_read_callback_default : forall 'n, 0 < 'n <= max_mem_access . (/* access type */ string, /* addr */ xlenbits, /* width */ int('n), /* value */ bits(8 * 'n)) -> unit
+function mem_read_callback_default (t, addr, width, value) = {
+  print_mem("mem[" ^ t ^ "," ^ BitStr(addr) ^ "] -> " ^ BitStr(value))
+}
+
+val xreg_write_callback_default : (regidx, xlenbits) -> unit
+function xreg_write_callback_default (reg, value) = {
+  print_reg("x" ^ dec_str(regidx_to_regno(reg)) ^ " <- " ^ BitStr(value))
+}
+
+val freg_write_callback_default : (regidx, flenbits) -> unit
+function freg_write_callback_default (reg, value) = {
+  /* todo: will only print bits; should we print in floating point format? */
+  print_reg("f" ^ dec_str(regidx_to_regno(reg)) ^ " <- " ^ BitStr(value))
+}


Now that the OCaml simulator is gone, I wonder if we should just move these default callbacks to C? Doing the sail -> C -> sail roundtrip seems a bit unnecessary?

I agree, it's done.

1. Added callback functions for XRegs, FRegs, VRegs, PC, CSR, memory writes and CSR, memory reads. 2. Added implementation of callbacks for RVFI records in riscv_rvfi_callbacks.c. 3. Added default callback OCaml-implementations in the file platform.ml.

…_callbacks.c.

…v_default_callbacks.c

kseniadobrovolskaya · 2024-10-29T14:02:40Z

@arichardson, @Timmmm, what do you think of the current version?

Timmmm reviewed Jun 11, 2024

View reviewed changes

model/riscv_insts_vext_vset.sail Outdated Show resolved Hide resolved

arichardson reviewed Jun 11, 2024

View reviewed changes

arichardson reviewed Jun 13, 2024

View reviewed changes

kseniadobrovolskaya force-pushed the Callbacks_for_state-changing_events branch 3 times, most recently from 10ae49b to 6e0f187 Compare July 1, 2024 12:55

Timmmm reviewed Jul 22, 2024

View reviewed changes

jrtc27 reviewed Jul 22, 2024

View reviewed changes

arichardson reviewed Jul 23, 2024

View reviewed changes

kseniadobrovolskaya force-pushed the Callbacks_for_state-changing_events branch 3 times, most recently from 09acccc to be696b8 Compare July 26, 2024 08:51

arichardson reviewed Jul 26, 2024

View reviewed changes

kseniadobrovolskaya force-pushed the Callbacks_for_state-changing_events branch 2 times, most recently from 98f3973 to 7dcbd56 Compare October 10, 2024 12:26

arichardson reviewed Oct 10, 2024

View reviewed changes

kseniadobrovolskaya force-pushed the Callbacks_for_state-changing_events branch from acbdacc to 779fbf7 Compare October 15, 2024 15:31

kseniadobrovolskaya added 3 commits October 17, 2024 13:21

Added implementation of callbacks for trace printing in riscv_default…

ffc1318

…_callbacks.c.

Added implementations of default callbacks for trace printing in risc…

232e5bd

…v_default_callbacks.c

kseniadobrovolskaya force-pushed the Callbacks_for_state-changing_events branch from 779fbf7 to 232e5bd Compare October 17, 2024 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing callbacks in sail-riscv for state-changing events #494

Implementing callbacks in sail-riscv for state-changing events #494

kseniadobrovolskaya commented Jun 11, 2024 •

edited

Loading

Timmmm left a comment

Timmmm commented Jun 11, 2024

arichardson Jun 11, 2024

Timmmm Jun 11, 2024

bacam Jun 12, 2024

Timmmm Jun 13, 2024

kseniadobrovolskaya Jun 13, 2024

Timmmm commented Jun 11, 2024

kseniadobrovolskaya commented Jun 13, 2024

Timmmm commented Jun 13, 2024

arichardson left a comment

Timmmm commented Jun 13, 2024

kseniadobrovolskaya commented Jul 16, 2024

Timmmm left a comment

Timmmm Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

Timmmm Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

Timmmm Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

Timmmm Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

Timmmm Jul 22, 2024

Timmmm Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

jrtc27 Jul 22, 2024

kseniadobrovolskaya Jul 24, 2024

arichardson left a comment

arichardson Jul 26, 2024

kseniadobrovolskaya Oct 10, 2024

arichardson left a comment

arichardson Oct 10, 2024

kseniadobrovolskaya Oct 15, 2024

kseniadobrovolskaya commented Oct 29, 2024

		@@ -340,8 +340,7 @@ function trap_handler(del_priv : Privilege, intr : bool, c : exc_code, pc : xlen

Implementing callbacks in sail-riscv for state-changing events #494

Are you sure you want to change the base?

Implementing callbacks in sail-riscv for state-changing events #494

Conversation

kseniadobrovolskaya commented Jun 11, 2024 • edited Loading

Timmmm left a comment

Choose a reason for hiding this comment

Timmmm commented Jun 11, 2024

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 commented Jun 11, 2024

kseniadobrovolskaya commented Jun 13, 2024

Timmmm commented Jun 13, 2024

arichardson left a comment

Choose a reason for hiding this comment

Timmmm commented Jun 13, 2024

kseniadobrovolskaya commented Jul 16, 2024

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

arichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseniadobrovolskaya commented Oct 29, 2024

kseniadobrovolskaya commented Jun 11, 2024 •

edited

Loading