riscv-non-isa / tg-nexus-trace

RISC-V Nexus Trace TG documentation and reference code
https://jira.riscv.org/browse/RVG-96
Creative Commons Attribution 4.0 International
43 stars 32 forks source link

Feedback on RISC-V N-Trace Specification 1.0.0 rc2.0 - chapter 7 #49

Closed ved-rivos closed 6 months ago

ved-rivos commented 7 months ago
  1. Ownership: The third bullet covers the case of instructions which may lead to privilege mode change. Further instructions - ecall, ebreak - that cause privilege mode change do not retire - they only raise an exception - so the first bullet "an instruction which is changing privilege mode retired" is not correct. Suggest fixing as follows:

    1. Upon the retirement of an instruction that writes to the scontext/hcontext CSR.
    2. Following any trace synchronizing message, specifically any message that includes the SYNC field. Importantly:
      • Should hcontext be implemented, the protocol requires two consecutive messages: the first presenting hcontext information and the second scontext information. This sequence is critical for enabling the decoder to precisely identify the code associated with a specific process.
    3. In the event of a trap or trap return that results in a change in privilege mode.
  2. Ownership: Is the requirement to send the hcontext first and scontext second a hard requirement? A decoder has to wait for both since with either order a decode cannot proceed before both have been received. Since the order is not material the second bullet could be rephrased to "requires two consecutive messages: one to present the hcontext information and a second to present the scontext information. The order of the two messages is not material but they must be generated consecutively."

  3. DirectBranch Section 7.2 - "This message is..." -> Qualifying with "direct" is not required and may be misleading as there are no indirect conditional branches. Suggest: "This message is generated exclusively in BTM mode, upon the retirement of a taken conditional branch." In rest of section "direct conditional branch" -> "conditional branch"

  4. Error: "It is suggested to have a timestamp at the moment when the first trace messages got dropped, but it is not required." - "It is recommended that the timestamp reported in the message corresponds to the moment when the first trace message was dropped; however, this is not a requirement." to make it clear that the timestamp referenced is the timestamp carried in the message.

  5. The note for Error Message states that the message must be produced even if trace is stopped. Is this problematic? If trTeEnable is cleared the encoder must not generate any messages. Generating messages when disabled may lead to unexpected behavior such as corrupting trace RAM.

  6. RepeatBranch: A statement that RepeatBranch must only follow one of the DirectBranch or IndirectBranch messages should be included.

  7. Is there a limit on B-CNT?

  8. What happens on a Debug Mode entry if HREPEAT counter was not 0 or B-CNT counter was not zero is not specified? Should encoder generate a RepeatBranch or ResourceFull message before sending the ProgTraceCorrelationMessage?

  9. Section 7.8 : "when the HIST mask or I-CNT counter has reached.." -> "when the HIST register is full or the I-CNT counter has reached maximum"

  10. Should section 7.8 title be "ResourceFull" instead of "Resource Full" to match format of name used for all other messages.

Editorial:

  1. "Original IEE..." -> "The IEEE-5001 Nexus Standard presents tables with TCODE (which is sent first) in the last row. In contrast, this specification describes Fields in Messages in the order they are sent (the first field sent is described first), aligning with the order of storage, processing, and text dumps."

  2. Ownership: "This message..." -> "This message furnishes the requisite context (privileged mode and Context ID, as assigned by the operating system or hypervisor), enabling the decoder to correlate program flow with distinct code segments associated with various programs. Activation of this feature is contingent upon the explicit enabling of the trTeContext control bit. Reporting of this information occurs under one of the following three conditions:"

  3. Ownership: "Bit layout can be defined.." -> "Bit layout is defined.."

  4. DirectBranch: "Next PC is determined.."-> "Next PC is determined by decoding the conditional branch to determine the encoded signed offset and adding it to the address of the conditional branch instruction."

  5. IndirectBranch: "This message is generated..." -> "This message is generated under two conditions: (1) when an instruction that causes an indirect unconditional control flow change has retired, and (2) when an interrupt or exception is delivered. This specification is applicable exclusively to BTM mode."

    "Last instruction..." -> "The last instruction within the code block(s), as specified by the I-CNT field, either represents an indirect unconditional control flow change (i.e., jump, call, or return) or this packet is generated in response to an exception or interrupt reported on the ingress port. The next PC is determined by applying the Address Compression rules to the U-ADDR field present in this message."

    "Not-taken direct..." -> "Not-taken conditional branches and direct unconditional jumps do not generate any trace; however, they do increase the I-CNT. Additionally, direct unconditional jumps modify the PC to the destination address specified in the instruction. Consequently, the PC of the last instruction in a code block(s) can be determined."

  6. Error: "Error message..."-> "An error message must be generated in the event of an internal messages FIFO overflow, resulting in the loss of a trace message."

    "FIFO overrun caused messages (one or..." -> "A FIFO overrun has resulted in the loss of one or more messages."

    "Implementation may always ..."-> "The field must be generated even if the reported value is 0, to guarantee that the TSTAMP field aligns at the byte boundary."

  7. ProgTraceSync: "This message is generated.." "This message is produced at the start or restart of trace. In such instances, the I-CNT field is required to be set to 0. However, under certain conditions associated with the SYNC parameter (e.g., External Trace Trigger), the I-CNT field may not be zero. Instead, it serves to pinpoint the precise Program Counter (PC) location at which the specified trigger or event occurred. Additionally, the F-ADDR field provides the complete PC address at the moment the trigger was activated."

  8. DirectBranchSync: "This message.."-> "This message is produced under the same conditions as the DirectBranch message. However, it further includes details on the reason for synchronization via the SYNC field, as well as the full Program Counter (PC) address through the F-ADDR field."

  9. Section 7.10 -> IndirectBranchHistHist -> IndirectBranchHist

  10. Resource Full "This message is ..."-> "This message is emitted when either the HIST register is full or the I-CNT counter reaches its maximum value for a given encoder implementation. This mechanism ensures that no information is lost, as it enables the decoder to reconstruct larger I-CNT and HIST fields by adding or concatenating the emitted values."

    "Not repeated HIST..."-> "When RCODE is set to 1, this signifies that the HIST register is full and will not be repeated. Under these circumstances, the HIST field generally encapsulates the maximum number of history bits implemented within the HIST register. Nonetheless, implementations may opt to include any quantity of history bits in this field, with the range extending from a minimum of 2 bits up to the maximum defined by NTRACE_MAX_HIST bits.

    Should the I-CNT counter and the HIST register simultaneously reach their respective capacity limits, it is mandatory to emit two successive ResourceFull messages."

  11. ProgTraceCorrelation "It provides..."-> "This message includes the EVCODE field, which specifies the reason for generating this message, alongside the I-CNT and HIST fields. These fields collectively enable the decoder to accurately identify the PC location where execution or tracing was halted."

mipsrobert commented 6 months ago

Addressed in 1.0.0_rc25 (or even earlier ...) version.