riscv-non-isa/tg-nexus-trace

Feedback on chapter 7 - RISC-V N-Trace (Nexus-based Trace) Specification

ved-rivos opened this issue · 1 comments

ved-rivos commented

  1. "messages is provided in Fields in Messages table above" is better to provide
    a cross reference to section number of table and avoid "above".
    Use same language/format as above in "Fields in Messages" section

  2. Section 7.1 - hypervisors perform context switch for the scontext. Will it be
    confusing for decoders if that happens? The encoder only has a context
    input. This section discussing hcontext and scontext could be confusing.
    This section should be worded with respect to the encoder inputs.
    A change in context value just depends on what the hart feeds in the context
    input to the TE, so this doesn't have to be scontext/hcontext write. Could also be
    a change in ASID/VMID, or even something else. Reword this to follow the language
    used for hart->trace-encoder input.

  3. Does Ownership get produced when the instruction executes or when it retires?
    If the SRET traps to M-mode due to TSR will that be considered as the SRET
    having executed and so produce the ownership message?

  4. "If hcontext is implemented two" -> "if hcontext is implemented then two"
    Since there is only a "context" input to the encoder how does it infer whether
    to send a first message with hcontext and second message with scontext.

  5. Section 7.2 - The explanation and notes are repeated many times through
    the specification. It may be less error prone and easier to comprehend if
    this chapter described the message formats and the later sections do the
    explanation on how to decode the message.

  6. "Immediately following any trace synchronization message" - this implies that
    there cannot be an idle flit between a trace synchronization message and an
    ownership message. Is that intentional? Or was the intention "As the next
    message following a synchronization message"?

  7. At entry and returns to/from exception and interrupts - this statement is
    ambigous. The return from an exception is accomplished by itype=3 classified
    instructions. Changing privilege is also only by these instructions and is not
    required to change privilege.
    Suggest rewording the three bullets as:

    1. When the context input to the encoder changes
    2. As the next message following a trace synchronization message.
    3. Following an interrupt or exception trap
    4. When an exception or interrupt return instruction is retired

  8. Table 11 - providing values in decimal and hexadecimal is redundant. Suggest
    using hexadecimal (or decimal) consistently.

  9. Table 12 - "V or PRV" -> "V and/or PRV"

  10. Section 7.1 - RTL syntax may not be intuitive to all readers. Consider
    a simpler notation that is more universally understood.

  11. Section 7.3 - missing period after first sentence.

  12. The (or interrupt/exception ..) is not in lieu of the the instruction
    retiring but seems implied by this text. Suggest breaking it out clearly.
    This message is generated when:
    a) an instruction causes an indirect unconditional control flow change
    b) an interrupt trap is delivered
    c) an exception trap is delivered
    The explanation that follows table 14 are repeating the text at start of this section.

  13. Section 7.3 - the explanation should remove "described by HIST" as this
    message is not applicable in HTM

  14. "Next PC....(either as F-ADDR or as U-ADDR)" is ambigous. This message
    only includes U-ADDR and U-ADDR are always in reference to last F-ADDR.
    It may also be best practice to avoid duplicating normative text.
    Since there is a full section on adddress compression, this text can
    simply state:
    "Next PC is determine by applying the Address Compression rules using the
    U-ADDR in field in this message"

  15. The informative note about not-taken direct conditional branches is also
    redundant repetition as that rule has been explained before.

  16. As the leading text in 7.3 also explains when this message is generated
    the repeat of that text in the explanation is not required

  17. Since all ETYPE code points are reserved for specific purposes, what the
    label Standard implies is unclear. Perhaps only 0-7 are standard and 8-15
    are for custom. Suggest
    "0 : Queue..."
    "1..7: Reserved for future standard extensions."
    "8..15: Designated for vendor defined errors"
    Reconsider reserving half the encoding space for custom use.

  18. Table 15. Why is it required for TSTAMP to be byte boundar aligned for
    Error message but such a constraint is not imposed for any other message?
    Is this constraint required? Further since TCODE + SRC + ETYPE can be
    variable, how does always producing ECODE enable byte alignment?

  19. Table 15. what do x in the ECODE mean? They can be 0 or 1 or are they
    required to be 0? This should be explained.

  20. ECODE field should be Fixed and not Var. Reword "standard:", "Reserved", "Vendor.."
    as suggested before for ETYPE

  21. Error message timestamp. Should this be timestamp of the first dropped
    message or last dropped message? The text implies "moment when ... dropped"
    seems to indicate last dropped message. Is that right?

  22. The statement that "Error message must be produced on a FIFO overflow"
    seems like wants to be a normative statement . The informative text box
    can then explain why.

  23. Table 16 - since F-ADDR is always provided the utility of I-CNT is not
    clear even for the sync caused by external trace trigger.

  24. Table 16 - is the F-ADDR when this sync is caused for extrnal trace trigger
    required to be that of the PC that caused the trigger to fire?

  25. Indirect Branch Sync - th eexplanation can be similar to Direct Branch SYnc
    and not repeat the reasons for indirect brach message. Same about the
    informational note (repeated third time).

  26. Table 19 - same comment about "Standard". Reconsider number of custom encodings.

  27. Table 19 - Is this message generated on the overflow or its generated on the resource
    being full. The text implies that it is on overflow and that there is a loss
    of recording that occurs. Why not report on full like the message name
    implies.

  28. Table 19 - Reserved for vendor... -> Designated for vendor...

  29. Table 19 - Should Opt be labelled Cfg?

  30. Section 7.8 - Explanation
    I-CNT - this one can be removed as it is repeated
    Not repeated HIST... -> this should be non-normative
    RCODE=2 -> integrate this text into the table.
    Add an explanation for why it is suggested that I-CNT should be reported first - what bad happens otherwise?

  31. Section 7.10 - to avoid repeatation - the explanation can state - this is
    generated when HTM is used and the rules used to generate IndirectBranchSync
    apply.

  32. Section 7.11 - the explanation should be updated - its not when an identical
    branch message is encountered but when B-CNT number of identical branch
    messages are encountered. Are all branch messages allowed to be collapsed or
    only those without SYNC?

  33. Table 23 - acronymn CDF used without explanation. Remove "IMPORTANT"
    and fix case of "IN"

  34. Table 23 - reword "even if HIST is empty=0x1"

mipsrobert commented

I am closing all N-Trace PDF related issues with same comments as all issues were handled via comments/discussions in SINGLE Google Docs. Relevant links are as follows:

Notes to N-Trace PDF: https://docs.google.com/document/d/1h__c0Kc7TQAWMh5bw9cNC9bl_IGqyY_ylPV14uc2xj0

N-Trace PDF rc20: 221f6b1
N-Trace for ARC review: 1de77dc