mipsrobert
commented
6 months ago 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: https://github.com/riscv-non-isa/tg-nexus-trace/commit/221f6b1cef94c5f503554e0cb0deb4046dd49686 N-Trace for ARC review: https://github.com/riscv-non-isa/tg-nexus-trace/commit/1de77dcff1a75197232f72b8fe6863892da0cc74
This issue summarizes the feedback and comments from AR on Chapter 3.
The specification states that 38 bytes is the maximum message size. It was not apparent that the protocol itself has such a limit on the maximum message size. The last row of the comparison table should be updated to state "In this version of the specification is 38 bytes (sum of all field lengths)" instead of "38 bytes (worst sum of all fields" - which would imply a inflexible upper bound fixed at 38 bytes. Same comment about the following sentence - "Max message size (38 bytes) " should be reworded as
"The maximum message size of 38 bytes in this version of the specification is to transmit the IndirectBranchHistSync message which includes TCODE/ SRC/ SYNC/ B-TYPE (5 bytes total), I-CNT(30 bits, 5 bytes), F-ADDR(63 bits, 11 bytes), HIST(32 bits, 6 bytes), and TSTAMP(64 bits, 11 bytes).
Did not include "worst" as assumption was that it just means maximum size. If there is a different meaning please clarify.
Reword "Particular hardware ...." "While implementations may have a shorter maximum message size (e.g, due to I-CNT being smaller), they must assure that the internal FIFOs are designed to hold at least two maximum sized messages that the implementation can produce." This does not make assumptions about the implementation being hardware or firmware or a combination of both.
Reword "Decoding software..." for better clarity "While decoding software may be designed to avoid dynamic memory allocation, it must nonetheless be robust enough to handle messages of any size. This is to account for scenarios where trace memory could be corrupted, such as a trace consisting entirely of zeros, which could be interpreted as an unusually long variable-length field." Or alternatively error out.
Section 3.1, "on LSB part of each byte" -> "located in the least signficant bits of each byte"
Section 3.1 Second sub-bullet of second bullet should be split into two. The sub-bullet should only describe the non-last bytes of a variable length field. A separate main bullet created for fixed-length fields.
The statement "initial parts of longer variable" may be misread to imply that the rule applies to only some initial bytes. Its better to write it as "The non-last bytes of a variable-length field are indicated by MSEO[1:0]=00"
Section 3.1 "The last byte of a variable-length.." - should be written to "The last byte of a variable-length field, that is not the last byte of a message, is indicated by MSEO[1:0]=01". Else it contradicts a bit with the later bullets. Similar wording can be used for the fixed-length fields rule.
Section 3.1 - "The last byte of a message..." - there is a stray ** in the text.
Table 5 - these transitions are not factoring in the Last byte of variable-length field also being last byte of message i.e. 00-11 (or 01) should be a end of variable length message.
Table 5 - Since the last byte of variable length message must be 11 if it is also last byte of message, the 01-11-(more 11s) transition is not allowed for End of message. Should it not just say "00-11-(more 11s)"
Table 5 - Idle is stated as 11s. It should be 11-11s?
Tabel 5 - message transmission can also see 00-01-00 and not just 00s
Section 3.1 - is there a unstated rule that first field of message cannot be a 6-bit variable-length field? If that is allowed then Start of message could also be 11s->00(or 01)? If it is disallowed then should state that rule.
Since the N-trace reproduces/copies parts of the IEEE-ISTO 5001-2012 - like section 3.1 - the N-trace specifcation should include copyright notice and the rules in the copyright are followed. Either please remove the reproductions or ensure that the document is compliant with the IEEE copyright rules.
Section 3.3 - "Idle state must be...all MSEO..." -> "...all 1s MSEO..."
Section 3.3 -> do not split "The following 4 example sequences:" and "are all valid"
The message transmission protocol of N-trace is different than that of Nexus Adding a diagram along the lines of Figure 5-2 of Nexust specification is good to highlight the divergence.
"LSB bits" is redundant. Better to just say "least significant bits"
"N-trace messages transmission protocol..." -> "N-trace message transmission protocol..." ->
What does "each LSB first" mean?
Table 5 - This table is confusing. I guess it is only listing the MSEO bits of each byte. Term "Message transmission" should be clarified.
Section 3.2 : ALWAYS -> always
Section 3.3 - All of this seems implicit from prior rules. Maybe just state the first bullet as a sentence in an earlier section, and remove this section?
Section 3.4 - Should this entire section be non-normative?
Table 6 - ICNT[10]=0x7D - Re-phrase the description.
Table 6 - Rephrase "Here we could have TSTAMP..."