Trigger match on csrrs/csrrc to tdata1 about riscv-debug-spec

[rtwfroody]

I had always assumed that csrrs/csrrc are just like csrw. Not sure what to make of the line you quote that "Other bits in the CSR are not explicitly written."

Regardless, I think the hit bit will be set. If timing=before, then the CSR isn't modified before the action is taken, so there's no conflict. If timing=after, then the trigger fires after the instruction has retired, so there's no conflict either. In both cases there's a clear ordering of the CSR being updated by the instruction, and the CSR being updated because the trigger fires. They don't happen simultaneously.

[tomaird]

Apologies I forgot to mention another important part of this example - the action being 2-4 (trace related) and the timing being "before" so the triggering instruction still executes.

If the action is 0-1 then it's more clear because the triggering instruction doesn't execute because we've entered debug mode or taken an exception. And if timing is "after" it's more clear as well because the triggering instruction has fully executed and retired before the trigger fires.

The quote I mentioned was because if the CSR instruction were to explicitly write all bits then it would be reasonable to assume that hit might be overwritten by the CSR instruction. But since these bits are not explicitly written I would expect them not to be overwritten by the CSR instruction.

I guess another important point that complicates things is that the spec allows instructions to partially execute before firing, even when timing=before. So it could be argued that it's reasonable for both HW (trigger) and SW (CSR instruction) updates to the CSR to happen simultaneously.

However I tend to agree with you that in my example the HW write to the CSR should happen first (when the trigger fires) and then (if the action is not 0-1) the instruction executes and does the SW update afterwards. With both CSR updates occurring independently.

[pdonahue-ventana]

I looked at Zicsr and I don't see the sentence about "Other bits in the CSR are not explicitly written." Where is that?

[tomaird]

Ah I was looking at an older version of the Unprivileged spec, it looks like that section has been reworded in a more recent version. Although I'm not sure if that really affects the discussion here.

[pdonahue-ventana]

I think that your concern is with this:

Some CSRs, such as the instructions-retired counter, instret, may be modified as side effects of
instruction execution. In these cases, if a CSR access instruction reads a CSR, it reads the value prior to
the execution of the instruction. If a CSR access instruction writes such a CSR, the write is done
instead of the increment.

If you have:

• an mcontrol6 execute address trigger with action=2 and hit=0 and tdata2=X
• tselect points to that trigger
• instruction at address X is a csrrc/csrrs to tdata1 where the hit bit is not part of the mask (so that bit would be unchanged by this software write)

One concern I have is that there is no "increment" as stated in the last sentence I quoted. If that should be read as "the software write is done instead of the hardware write" then I think that the hit bit should not be set in the above situation because setting hit is a side effect. If the sentence literally only applies to CSRs that are written as side effects of instruction execution where such side effects involve incrementing the CSR, then the hit bit probably should be set. I assume that it was not meant to be limited to incrementing.

@aswaterman: You have a perspective on the history of Zicsr. What are your thoughts?

[aswaterman]

The text wasn't meant to be limiting to incrementing; that choice of word was in the context of the instret example. My reading of the spec concurs with your first interpretation, @pdonahue-ventana.

[pdonahue-ventana]

Thanks. That's what I figured. The Zicsr text should probably change the word "increment" to something more generic.

[aswaterman]

@pdonahue-ventana What do you think about "the explicit write is done instead of the update from the side effect"?

[pdonahue-ventana]

That sounds good.

[aswaterman]

riscv/riscv-isa-manual@8c2e1ae

[pdonahue-ventana]

That looks good. (Frankly, I'm not a big fan of this rule. But setting that aside, the change looks good from the point of view of clarifying the intent.)

[aswaterman]

I'm not planning on proposing changing anything, but out of curiosity, what would you have preferred: invert the order so that the write happens first, and the side-effecting update considers the new value of the register; or make it unspecified which order they happen in? (I can't think of a case where it matters much, to be honest...)

[pdonahue-ventana]

The current requirement leads to hacks like https://github.com/riscv-software-src/riscv-isa-sim/blob/master/riscv/csrs.cc#L1042 in Spike and similar unnatural tricks in hardware. It seems more natural for the CSR write to occur during the execution of the instruction and the side effect to happen after the instruction (seeing the new value). That would obviously be a (verboten) normative change to Zicsr because it causes a csrw minstret; csrr minstret to see a value of 1 instead of 0. It seems to me that 0 instructions would mean that nothing happened at all. But there actually was one instruction (or maybe the second half of one and the first half of another which adds up to 1).