Implicit CSR writes are not logged

[allenjbaum]

Some implicit CSR updates (e.g. updated Mstatus when trapping into Mmode, or MTVAL, etc) are logged,
but most or not.

I would propose that no further CSRs be allowed to be merged unless implicit accesses
(if there are any - this occurs primarily for status fields, rather than control fields)
are logged (one line/CSR, prexise format TBD, but should be uniform)).

Eventually, this has to be extended already existing status CSR fields (as opposed to control and readonly fields)

[Timmmm]

I think this is a duplicate of #184

[allenjbaum]

Yes, but using much stronger language. This is required for us to measure coverage, and is a fairly strong argument why we can't use the Sail model compared to other existing solutions.

[Alasdair]

Do you have a description of the desired format you require?

[allenjbaum]

The format would be the same as what is currently logged : CSR {CSR_Name} <- {Hex-Value}

[Alasdair]

Maybe the way to go would be to add a CSR attribute to the appropriate registers, then have Sail insert the logging statements. Would perhaps be less error prone than having to manually write them in the source, and shouldn't be too hard to implement.

[allenjbaum]

Sorry, I don't understand. Which appropriate register needs to have a CSR attribute added? The idea is to expose this architectural state update for any and all CSRs that are modified by any instruction (and/or any external event eventually)

…

On Tue, May 7, 2024 at 6:00 PM Alasdair Armstrong ***@***.***> wrote: Maybe the way to go would be to add a CSR attribute to the appropriate registers, then have Sail insert the logging statements. Would perhaps be less error prone than having to manually write them in the source, and shouldn't be too hard to implement. — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJR5GLI3W3VGFGTQ463ZBF2MRAVCNFSM6AAAAABGGN7IUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZGU2DANZTGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Alasdair]

I mean if we know what registers are CSRs then rather than writing something like print("CSR " ^ reg_name(...) ^ " <- " ^ ...) in the spec, we can just automatically insert the correct logging statements. That way we never miss any.

[allenjbaum]

We do know which registers are CSRs; there are potentially 4096 of them in the CSR address space, and they are explicitly the source or target of CSRR[W/S/C][I] ops. What I want is what you describe: anytime a CSR field is updated in Sail, either implicitly or explcitly, it automatically gets loggedn( e.g. the print statement above) This is along with any other non-CSR register updates that are logged as Rd (e.g. X1..31, or F0..31). There are a small number of whose implicit updates that are currently logged (e.g. xTVAL during a trap), and all of them are logged by explicit CSR op writes.

…

On Tue, May 7, 2024 at 6:10 PM Alasdair Armstrong ***@***.***> wrote: I mean if we know what registers are CSRs then rather than writing something like print("CSR " ^ reg_name(...) ^ " <- " ^ ...) in the spec, we can just automatically insert the correct logging statements. That way we never miss any. — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJUJA3HNDBQUHAX2IZTZBF3O3AVCNFSM6AAAAABGGN7IUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZGU2DOMRUG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jrtc27]

That doesn't help you when the model, as it should for clarity, accesses the register directly by its language-level register, which has zero ties to any notion of CSR address space. They are indistinguishable from general-purpose registers in that regard.

[allenjbaum]

OK, I think I understand the context now.
So, how would you add the CSR attribute to (all, preferably) CSRs, and when you did,
how do you get Sail to automatically out[ut a logging string if any field of the CSR is updated?

[Timmmm]

Maybe the way to go would be to add a CSR attribute to the appropriate registers, then have Sail insert the logging statements. Would perhaps be less error prone than having to manually write them in the source, and shouldn't be too hard to implement.

I don't think this would work unfortunately. E.g. we want to log writes to sstatus but that register doesn't explicitly exist in the model. Our current solution is this:

    writeCSR(csr, new_val);
    // Immediately call this after writing the CSR.
    track_writeCSR(csr);

// Add it to a list of CSR writes - though in retrospect I like the callback solution better.
function track_writeCSR(csr : csreg) -> unit = {
  track_csr_writes = struct {
    index = csr,
    value = readCSR(csr), // Note, that this is a bit janky since in theory CSR reads can have side effects. In practice they don't though.
  } :: track_csr_writes;
}

And then we add a handful of extra calls:

riscv_fdext_regs.sail:  track_writeCSR(csr_name_map("mstatus"));  // dirty_fd_context
riscv_fdext_regs.sail:  track_writeCSR(csr_name_map("fcsr")); // accrue_fflags
riscv_sdext_control.sail:  track_writeCSR(csr_name_map("mstatus")); // leave_debug_mode
riscv_sys_control.sail:      track_writeCSR(csr_name_map("mstatus")); // mret
riscv_sys_control.sail:      track_writeCSR(csr_name_map("mstatus")); // sret
riscv_sys_control.sail:      track_writeCSR(csr_name_map("mstatus")); // uret

As far as I know that covers all the implicit CSR writes (maybe not in Vector). Given there are so few I think it's fine to just do it manually. We should just do it as a callback, as suggested in #184.

[allenjbaum]

Hmm: I thought that side effects on reads to CSRs were forbidden (explicitly) in the spec. The CSRs that don't exist are usually labeled as "restricted view" in the priv spec. But, unless they also contain bits that aren't in mstatus, you could simply log it as a write to mstatus instead using the name of the "alternate" view. That will be good enough for coverage.

…

On Wed, May 8, 2024 at 1:01 AM Tim Hutt ***@***.***> wrote: Maybe the way to go would be to add a CSR attribute to the appropriate registers, then have Sail insert the logging statements. Would perhaps be less error prone than having to manually write them in the source, and shouldn't be too hard to implement. I don't think this would work unfortunately. E.g. we want to log writes to sstatus but that register doesn't explicitly exist in the model. Our current solution is this: writeCSR(csr, new_val); // Immediately call this after writing the CSR. track_writeCSR(csr); // Add it to a list of CSR writes - though in retrospect I like the callback solution better. function track_writeCSR(csr : csreg) -> unit = { track_csr_writes = struct { index = csr, value = readCSR(csr), // Note, that this is a bit janky since in theory CSR reads can have side effects. In practice they don't though. } :: track_csr_writes; } And then we add a handful of extra calls: riscv_fdext_regs.sail: track_writeCSR(csr_name_map("mstatus")); // dirty_fd_context riscv_fdext_regs.sail: track_writeCSR(csr_name_map("fcsr")); // accrue_fflags riscv_sdext_control.sail: track_writeCSR(csr_name_map("mstatus")); // leave_debug_mode riscv_sys_control.sail: track_writeCSR(csr_name_map("mstatus")); // mret riscv_sys_control.sail: track_writeCSR(csr_name_map("mstatus")); // sret riscv_sys_control.sail: track_writeCSR(csr_name_map("mstatus")); // uret As far as I know that covers all the implicit CSR writes (maybe not in Vector). Given there are so few I think it's fine to just do it manually. We should just do it as a callback, as suggested in #184 <#184>. — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJROGQTNFF5MLVGR3TTZBHLW3AVCNFSM6AAAAABGGN7IUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZHE4DGNZTGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Timmmm]

We're using this for model checking - not coverage - so it needs to exactly match what our DUT says. When you write to sstatus the instrumentation of our DUT reports it as a write to sstatus, not mstatus. Theoretically that could be changed but I think it would be much worse for debugging if we did, and I can't see any reason to given how easy it is to solve this issue manually.

I thought that side effects on reads to CSRs were forbidden (explicitly) in the spec.

It's not forbidden, it just happens to be the case that all existing standard CSRs don't have side effects on read (because that would be mad). But custom CSRs are allowed to:

Standard CSRs do not have any side effects on reads. Standard CSRs may have side effects on writes. Custom extensions might add CSRs for which accesses have side effects on either reads or writes.

So in practice it's not an issue for the model since we don't have any crazy CSRs like that. There's a comment in the existing code actually for one of these side-effect cases:

    let csr_val = readCSR(csr); /* could have side-effects, so technically shouldn't perform for CSRW[I] with rd == 0 */

But yeah it currently doesn't matter (and is unlikely to ever matter really).

[rsnikhil]

. I thought that side effects on reads to CSRs were forbidden (explicitly) in the spec.

In addition to the text cited by @Timmmm there is also this text, in the first para of 9.1 (Zicsr):

If rd=x0, then the instruction shall not read the CSR and shall
not cause any of the side effects that might occur on a CSR read.

[jrtc27]

Your model might want sstatus but someone else’s might want mstatus. Canonicalising makes things simpler to compare, and avoids confusion around whether state is aliasing or separate. I would argue mstatus should be reported since that is the actual underlying architectural state being changed. See also fflags+frm vs fcsr.

[Timmmm]

Hmm I suppose there are two similar but subtly different things you might want to know:

• What are the changes to the underlying architectural state? (mstatus changed)
• What action did the instruction perform? (wrote to sstatus)

There's actually an even more annoying subtlety we ran into: what about implicit CSR updates that don't actually change the value? Especially with implicit updates to fflags, you have a few options for when to record an update:

• Whenever it is written to (i.e. when any instruction that theoretically could change it is executed).
• Whenever its actual value changes.
• Whenever the flag result of an instruction is non-zero, even if ORing that into fflags wouldn't change its value.

That was a right pain to deal with.

Maybe the main issue with canonicalising is that it's going to be a lot more work than the 5 line change we have.

[allenjbaum]

From the point of view of coverage and arch test: we don't care what custom CSRs do because we steer clear of them. I know I have proposed side-effect on reads to simplify an extension definition, andit was shot down because of the read side-effect (it was basically a push/pop kind of op) So, if it is isn't actually speced, I doubt you'll see one)

…

On Wed, May 8, 2024 at 12:57 PM Tim Hutt ***@***.***> wrote: We're using this for model checking - not coverage - so it needs to exactly match what our DUT says. When you write to sstatus the instrumentation of our DUT reports it as a write to sstatus, not mstatus. Theoretically that could be changed but I think it would be much worse for debugging if we did, and I can't see any reason to given how easy it is to solve this issue manually. I thought that side effects on reads to CSRs were forbidden (explicitly) in the spec. It's not forbidden, it just happens to be the case that all existing standard CSRs don't have side effects on read (because that would be mad). But custom CSRs are allowed to: Standard CSRs do not have any side effects on reads. Standard CSRs may have side effects on writes. Custom extensions might add CSRs for which accesses have side effects on either reads or writes. So in practice it's not an issue for the model since we don't have any crazy CSRs like that. There's a comment in the existing code actually for one of these side-effect cases: let csr_val = readCSR(csr); /* could have side-effects, so technically shouldn't perform for CSRW[I] with rd == 0 */ But yeah it currently doesn't matter (and is unlikely to ever matter really). — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJRQO2TKV6UYZPS7MMLZBJ7RRAVCNFSM6AAAAABGGN7IUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBRGMZDGOBXGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[allenjbaum]

From the point of view of arch-test (Again) - I don't think canonicalizing matters as far as coverage is concerned; we can define the coverage to use mstatus or sstatus, we just need to know the rule. It will get a LOT more interesting when hypervisor is implemented, and CSRs effectively get swapped from one mode to the next. On Sat, May 11, 2024 at 12:06 AM Allen Baum ***@***.***> wrote:

…

From the point of view of coverage and arch test: we don't care what custom CSRs do because we steer clear of them. I know I have proposed side-effect on reads to simplify an extension definition, andit was shot down because of the read side-effect (it was basically a push/pop kind of op) So, if it is isn't actually speced, I doubt you'll see one) On Wed, May 8, 2024 at 12:57 PM Tim Hutt ***@***.***> wrote: > We're using this for model checking - not coverage - so it needs to > exactly match what our DUT says. When you write to sstatus the > instrumentation of our DUT reports it as a write to sstatus, not mstatus. > Theoretically that could be changed but I think it would be much worse for > debugging if we did, and I can't see any reason to given how easy it is to > solve this issue manually. > > I thought that side effects on reads to CSRs were forbidden (explicitly) > in the spec. > > It's not forbidden, it just happens to be the case that all existing > standard CSRs don't have side effects on read (because that would be mad). > But custom CSRs are allowed to: > > Standard CSRs do not have any side effects on reads. Standard CSRs may > have side effects on writes. Custom extensions might add CSRs for which > accesses have side effects on either reads or writes. > > So in practice it's not an issue for the model since we don't have any > crazy CSRs like that. There's a comment in the existing code actually for > one of these side-effect cases: > > let csr_val = readCSR(csr); /* could have side-effects, so technically shouldn't perform for CSRW[I] with rd == 0 */ > > But yeah it currently doesn't matter (and is unlikely to ever matter > really). > > — > Reply to this email directly, view it on GitHub > <#450 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHPXVJRQO2TKV6UYZPS7MMLZBJ7RRAVCNFSM6AAAAABGGN7IUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBRGMZDGOBXGY> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >