Implement callbacks for state-changing events

[kseniadobrovolskaya]

Add a callback API for register, PC and CSR updates, memory writes and traps. This is used for logging and RVFI instead of embedding that code in the model itself.

[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.

[Timmmm]

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]

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) = ... ?

[Timmmm]

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.

[bacam]

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

[Timmmm]

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!

[kseniadobrovolskaya]

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]

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]

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]

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]

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]

@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.

[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]

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

[kseniadobrovolskaya]

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

[Timmmm]

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.

[pmundkur]

A nice improvement, moving most of RVFI handling out of the main Sail model.

[pmundkur]

Other than the c-preserve and pc_write_callback, looks great!

[Timmmm]

LGTM. Just a couple of minor things and I think we should add some comments to either riscv_callbacks.h or riscv_callbacks.sail to say exactly what all the callbacks mean, because there are some subtleties. E.g. when exactly is trap_callback() called? Are addresses physical or virtual? How is mstatush etc. handled?

There are probably some other minor things we can improve but I think this is already a huge improvement so lets get it in!

[Timmmm]

LGTM. We can add the comments later. I imagine the exact API may change slightly when we try to use it in anger anyway.

[Timmmm]

Oh you missed a paddr though...

[jordancarlin]

Oops. Fixed. I added some comments to riscv_callbacks.h.

[Timmmm]

Nice work everyone!