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.

model/riscv_fdext_regs.sail

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.

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

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

Timmmm · 2024-11-29T12:15:41Z

Hi, sorry for not getting around to this. Just wanted to say it's still on my todo list and I will get to it!

Timmmm

Finally got around to this again - sorry for the delay!

I think it's pretty close - the main thing is merging riscv_rvfi_callbacks.c into riscv_default_callbacks.c.

You could also rename it to just riscv_callbacks.c.

Timmmm · 2024-12-16T14:34:47Z

Makefile

@@ -225,7 +225,7 @@ c_preserve_fns=-c_preserve _set_Misa_C

 generated_definitions/c/riscv_model_$(ARCH).c: $(SAIL_SRCS) model/main.sail Makefile
 	mkdir -p generated_definitions/c
-	$(SAIL) $(SAIL_FLAGS) $(c_preserve_fns) -O -Oconstant_fold -memo_z3 -c -c_include riscv_prelude.h -c_include riscv_platform.h -c_no_main $(SAIL_SRCS) model/main.sail -o $(basename $@)
+	$(SAIL) $(SAIL_FLAGS) $(c_preserve_fns) -O -Oconstant_fold -memo_z3 -c -c_include riscv_prelude.h -c_include riscv_platform.h -c_no_main $(SAIL_SRCS) -c_include riscv_default_callbacks.c model/main.sail -o $(basename $@)


I don't think we want to #include a .c file. This should include a header.

Timmmm · 2024-12-16T14:36:57Z

c_emulator/riscv_config.h

@@ -6,3 +6,4 @@ extern bool config_print_step;
 extern bool config_print_reg;
 extern bool config_print_mem_access;
 extern bool config_print_platform;
+extern bool rv_enable_callbacks;


I'd call this config_enable_rvfi instead.

Timmmm · 2024-12-16T14:38:19Z

c_emulator/riscv_rvfi_callbacks.c

@@ -0,0 +1,38 @@
+#include "riscv_config.h"
+
+int zrvfi_write(uint64_t addr, int64_t width, lbits value);


In future these should be in C, not Sail. I think it's probably best to do in a future PR but can you add

// TODO: Move these implementations to C.

Timmmm · 2024-12-16T14:43:16Z

c_emulator/riscv_rvfi_callbacks.c

+int mem_read_callback(const char *type, uint64_t addr, uint64_t width,
+                      lbits value)
+{
+  if (rv_enable_callbacks) {


We want a single callback that does both the printing, and the RVFI stuff (if enabled). So the contents of this file should be merged into riscv_default_callbacks.c. Something like this:

int mem_write_callback(uint64_t addr, uint64_t width, lbits value) { if (config_print_mem_access) { char *lbits_data = get_lbits_data(value); printf("mem[0x%.16lX] <- 0x", addr); for (int i = width - 1; i >= 0; --i) printf("%02hhX", lbits_data[i]); printf("\n"); free(lbits_data); } #ifdef RVFI_DII if (config_enable_rvfi) { sail_int len; CREATE(sail_int)(&len); CONVERT_OF(sail_int, mach_int)(&len, width); zrvfi_read(addr, len, value); KILL(sail_int)(&len); } #endif }

In future we should remove the #ifdef so RVFI_DII support is always enabled. It's unnecessary to have it as a compile time option - it just doubles compilation time. (But don't do it in this PR.)

This is probably the biggest blocker. Enabling RVFI should not disable printing.

Timmmm · 2024-12-16T14:43:51Z

c_emulator/riscv_sail.h

@@ -50,6 +50,20 @@ extern bool zhtif_done;
 extern mach_bits zhtif_exit_code;
 extern bool have_exception;

+/* Callbacks for state-changing events */


As mentioned above - move these into their own header and -c_include that.

Timmmm · 2024-12-16T14:46:30Z

c_emulator/riscv_default_callbacks.c

+int freg_write_callback(unsigned reg, uint64_t value)
+{
+  /* TODO: will only print bits; should we print in floating point format? */
+  if (config_print_reg)


Minor, but can you always use braces - single-statement blocks a bit of a footgun in C.

I thought we had clang-format set up to enforce that but maybe not. I'll double check that.

jordancarlin · 2024-12-16T04:56:02Z

model/riscv_csr_begin.sail

@@ -338,3 +338,4 @@ scattered function read_CSR
 /* returns new value (after legalisation) if the CSR is defined */
 val write_CSR : (csreg, xlenbits) -> xlenbits
 scattered function write_CSR
+


Not sure why a new line is being added to this file with no other changes.

Timmmm · 2025-01-06T09:26:55Z

@kseniadobrovolskaya would it be ok if I did a bit of work on this and pushed to this branch?

kseniadobrovolskaya · 2025-01-22T12:19:27Z

@kseniadobrovolskaya would it be ok if I did a bit of work on this and pushed to this branch?

Yes, of course.

kseniadobrovolskaya · 2025-01-22T14:39:01Z

@kseniadobrovolskaya would it be ok if I did a bit of work on this and pushed to this branch?

Hi @Timmmm, I just want to make sure that there is no misunderstanding - do you imply that you going to fix the pending comments from you, or should I continue to work on them?

jordancarlin · 2025-02-03T23:02:18Z

@Timmmm @kseniadobrovolskaya Any updates on this?

Timmmm · 2025-02-04T17:54:40Z

Any updates on this?

do you imply that you going to fix the pending comments from you, or should I continue to work on them?

Yes, at least that was my plan, but I haven't had any time to work on it yet! I'll rebase it now at least...

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. Added implementation of callbacks for trace printing in riscv_default_callbacks.c. Added implementations of default callbacks for trace printing in riscv_default_callbacks.c

Timmmm · 2025-02-04T18:14:27Z

I squashed, rebased and commented out mstatus tracking since it doesn't compile currently.

Also there are linker errors because the C file is no longer compiled in (we don't want to --c-include it). I'll take a look tomorrow.

* Combine the callback functions - we have one callback that does everything now. * Add a header for the callbacks that is included in the model C. * Fix some of the types.

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

model/riscv_fdext_regs.sail Show resolved Hide resolved

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 2 times, most recently from 779fbf7 to 232e5bd Compare October 17, 2024 10:54

Timmmm requested changes Dec 16, 2024

View reviewed changes

This was referenced Dec 16, 2024

Make RVFI a runtime option #642

Open

Move RVFI protocol implementation to C #643

Open

jordancarlin requested changes Dec 16, 2024

View reviewed changes

Timmmm mentioned this pull request Dec 18, 2024

Implement CMake build system #647

Merged

jordancarlin self-requested a review December 30, 2024 04:08

Timmmm added 3 commits February 4, 2025 18:09

Comment out mstatus tracking for now due to compilation errors

7ccaada

Fix precommit

14542bc

Timmmm force-pushed the Callbacks_for_state-changing_events branch from 232e5bd to 14542bc Compare February 4, 2025 18:13

Combine callback files and fix compilation

0679f52

* Combine the callback functions - we have one callback that does everything now. * Add a header for the callbacks that is included in the model C. * Fix some of the types.

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

		@@ -0,0 +1,38 @@
		#include "riscv_config.h"

		int zrvfi_write(uint64_t addr, int64_t width, lbits value);

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

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

Timmmm commented Nov 29, 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

Timmmm commented Jan 6, 2025

kseniadobrovolskaya commented Jan 22, 2025

kseniadobrovolskaya commented Jan 22, 2025

jordancarlin commented Feb 3, 2025

Timmmm commented Feb 4, 2025

Timmmm commented Feb 4, 2025

kseniadobrovolskaya commented Jun 11, 2024 •

edited

Loading