`sberror` not clearing properly causes OpenOCD interoperability issues

[timothytrippel]

The OpenTitan project uses GDB/OpenOCD to develop top-level debug and manufacturing test cases. We have noticed some test failures when performing unaligned system bus accesses (see here).

In short, when an unaligned read operation is performed, sberror is set to 3 (which is correct according to the spec), but after, OpenOCD tries to clear the error with a W1C, by sending 0x7, however, the sberror field fails to clear. We believe there is an RTL bug described here.

Could someone confirm / fix?

[andreaskurth]

I agree that writing 0x7 should clear the entire sberror field:

The RISC-V External Debug Support spec defines sberror as follows (excerpt from version 0.13.2, but version 1.0-STABLE does not differ in aspects relevant for this): and R/W1C as

Thus, according to "For each bit in the field, writing 1 clears that bit", 0x7 should indeed clear the entire sberror field.

---

What should happen when the sberror field (or a W1C field in general) is only partially written, though? The v0.13.2 spec does not define this. v1.0 clearly states that partial writes are not what R/W1C fields are intended for: and

---

In conclusion, I suggest changing https://github.com/pulp-platform/riscv-dbg/blob/09c8d7a4e67f6bde756c4de6319d87e929faf188/src/dm_csrs.sv?plain=1#L466 to

sbcs_d.sberror     = (|sbcs.sberror) ? 3'b0 : sbcs_q.sberror;

so that

• writing 0x7 clears sberror, which fixes the incompatibility with OpenOCD and the RISC-V Debug Spec;
• writing 0x1 also clears sberror, which maintains compatibility with current behavior and is allowed by the RISC-V Debug Spec ("The result of other writes is undefined.");
• writing any other non-zero value also clears sberror, which is allowed by the RISC-V Debug Spec ("The result of other writes is undefined.").

If compatibility with current behavior is not a concern, an alternative would be to clear sberror only if all bits are written.

Not clearing all bits on a partial write would not be meaningful IMO, because it modifies the error condition and still does not allow debug accesses.

[bluewww]

I agree with @andreaskurth interpretation of the spec. I would opt for the backwards compatible solution because we might already rely on this behavior in some testbench (assuming the meaning of undefined is the same as in e.g. the C programming language standard)

[andreaskurth]

Thanks for your feedback, @bluewww. :+1:

I did not find a definition of undefined in the RISC-V Debug Spec, and other RISC-V specs I know use implementation-defined instead. But I think interpreting undefined as implementation-defined for the Debug Spec, too, makes sense.

I'll create a PR to implement the backwards-compatible solution