riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
407 stars 147 forks source link

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

Open kseniadobrovolskaia opened 2 months ago

kseniadobrovolskaia commented 2 months ago

Hello!

I am reaching out to discuss some enhancements we are considering for our RISC-V processors test generator. Currently, we are utilizing SAIL as our golden model. To streamline our workflow, we believe it would greatly benefit our project to receive notifications when the model undergoes changes in its state.

Our proposed solution involves implementing callbacks, which would involve invoking functions via pointers whenever a specified change occurs within the model's state. Specifically, we are seeking to incorporate the following function declarations within the SAIL code:

Type Definitions:

Callback handler:   typedef struct callback_handler callback_handler;
Callback Functions:
    mem_update_callback_t
    xreg_update_callback_t
    freg_update_callback_t
    vreg_update_callback_t
    pc_update_callback_t

Following the implementation of these declarations, any user would have the ability to furnish their own implementation of the handler and associated functions. Consequently, this would enable immediate actions to be executed upon any state changes within the model.

In order to integrate this functionality, we would require calls to these functions (if defined) at the time of the changes within the following function definitions:

void zphys_mem_write(struct zMemoryOpResultzIozK *zcbz3590, enum zwrite_kind zwk, uint64_t zpaddr, int64_t zwidth, lbits zdata, unit zmeta)
unit zwX(int64_t zr, uint64_t zin_v)
unit zwF(int64_t zr, uint64_t zin_v)
unit zwrite_single_vreg(sail_int znum_elem, sail_int zSEW, uint64_t zvrid, zz5vecz8z5bvz9 zv)
unit zset_next_pc(uint64_t zpc)

Could you please provide your feedback on the feasibility of incorporating this functionality into sail-riscv? We believe it could significantly enhance our testing capabilities, but we would greatly appreciate your insights and guidance on this matter.

Thank you very much for your time and consideration.

Best regards, Ksenia Dobrovolskaya.

Timmmm commented 1 month ago

I agree we need a proper interface for this. Currently the only option is the RVFI code. We have hackily commented all that out and replaced it with our own RVFI-alike.

I'm not really a fan of using callbacks in general but it does nicely avoid some problems with the RVFI approach which I've run into - especially when multiple things can happen in one instruction (e.g. multiple CSR writes). So yeah I think this is a good idea.

The way to do it would be to add external Sail functions like this:

val record_writeCSR = pure {c: "record_writeCSR"} : (csreg, xlenbits) -> unit;

and then you must provide a default implementation of them. This would mean you could strip out all the existing hard-coded RVFI and logging code from the model which would nicely simplify it. E.g. when writing registers (wX) instead of

  if (r != 0) then {
     rvfi_wX(r, in_v);
     if   get_config_print_reg()
     then print_reg("x" ^ dec_str(r) ^ " <- " ^ RegStr(v));
  }

just have

  if (r != 0) then {
    record_wX(r, in_v);
  }
kseniadobrovolskaia commented 1 month ago

Hello!

I think that the function call approach has disadvantages. The thing is that depending on whether you need logs or not, you will have to replace the default implementation of the function and recompile the simulator.

I propose the following solution to this problem: at each step, we can add all the logs to the lists that users can access. NOTE: This is how the logging mechanism is implemented in Spike. That is:

  1. Define the following variables in Sail and put them in the top-level interface (c_emulator/riscv_sail.h): 1.1. rv_enable_logs : bool - Indicates whether state changes are to be logged. It is set to false by default, but users can change it, if necessary. 1.2. log_reg_write : list(log_mem_t) - List of register changes. 1.3. log_mem_write : list(log_mem_t) - List of memory writes. 1.4. log_mem_read : list(log_reg_t) - List of reads from memory.

Users can then read these lists and process them as desired.

  1. In sail-riscv if rv_enable_logs is set: 2.1 Add a log to the corresponding list at each state change. 2.2. Before each sail_step, reset all changes with log_reset.

This mechanism will allow logging several changes that occurred in one instruction, and logging nothing if rv_enable_logs is not set by the user. I have a draft implementation of the above approach. I can create a MR if no objections against the approach follows.

Timmmm commented 1 month ago

That is how we currently record CSR accesses but I think the "list of events" approach is likely worse than function callbacks.

  1. Lists are an endangered Sail feature. They may get removed one day since they're not synthesizable.
  2. You lose context and ordering information.
  3. Callbacks are an official Sail interface. Reading Sail lists from C is definitely not!

I don't understand your point about having to recompile the simulator. If you want to do multiple different things based on runtime settings just check those settings in your callback.