search

riscv / riscv-debug-spec

Working Draft of the RISC-V Debug Specification Standard
https://jira.riscv.org/browse/RVG-94
Other
459 stars 92 forks source link

Inconsistency in setting `cmderr` for different registers #1072

Open stefano-codasip opened 1 month ago

stefano-codasip commented 1 month ago

For abstractcs, abstractauto and command, the RISC-V Debug spec reads:

Writing this register while an abstract command is executing causes cmderr to become 1 (busy) once the command completes (busy becomes 0)

For dataX and progbufX the RISC-V Debug spec reads:

Accessing these registers while an abstract command is executing causes cmderr to be set to 1 (busy) if it is 0.

The former indicates that the Debug Module should wait for abstractcs.busy to become zero before setting abstractcs.cmderr to one. The latter seem to indicate an immediate setting of abstractcs.cmderr without waiting for the command to complete.

  1. Which behavior is the correct one?
  2. If the correct behavior is the one stated under abstractcs, abstractauto and command registers, then I believe there is a further inconsistency / issue with following sentence of the spec:

    If the debugger starts a new command while busy is set, cmderr becomes 1 (busy), the currently executing command still gets to run to completion, but any error generated by the currently executing command is lost.

If we wait for the command to complete before setting cmderr to one, and we get an exception (cmderr value 3), what should cmderr be set to in that case? Because the busy error occurs before the exception, but cmderr would still be zero when the exception comes in.

stefano-codasip commented 1 month ago

fyi @rtwfroody @pdonahue-ventana

en-sc commented 1 month ago
  1. Which behavior is the correct one?

AFAIU, this does not matter since [3.14.6. Abstract Control and Status (abstractcs, at 0x16), cmderr field description] states:

This field only contains a valid value if busy is 0.

As for your second point, my understanding is cmderr should be set to busy, as per:

any error generated by the currently executing command is lost.

If I'm correct, maybe rewriting the rule as:

If the debugger starts a new command while busy is set, the currently executing command still gets to run to completion, cmderr becomes 1 (busy) and any error generated by the currently executing command is lost.

Will make it a bit clearer.

JanMatCodasip commented 1 month ago

en-sc: this does not matter since (...) cmderr field description states:

This field only contains a valid value if busy is 0.

@en-sc Thank you for pointing this out, I missed this part of the spec initially. So cmderr may be arbitrary while busy=1(and should not be interpreted by the external debugger).

We discussed this with @stefano-codasip internally and prepared a proposal to clarify the wording in the spec: https://github.com/riscv/riscv-debug-spec/pull/1076

  1. Reword what happens when debugger starts a new abstract command while busy is set. (We used similar wording to what @en-sc suggested above.)
  2. Unify how the "cmderr busy" requirement is phrased - use the same sentence in all places.

Please, let us know if the proposed change looks OK to you. Thanks.

stefano-codasip commented 1 month ago

Thanks @en-sc - I also totally missed

This field only contains a valid value if busy is 0.

As @JanMatCodasip pointed out, we discussed this internally, and both agree on the changes proposed in his pull request.