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

Question

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

ved-rivos opened this issue 9 months ago · 1 comments

. Chapter 8 - "Only execution addresses (as seen by the hart)" - reword
Only the pc is reported.

Chapter 8 - "In case MMU..." -> "When virtual memory system is enabled"
"Address fields are being sent..." -> "The address field does not include the
bit 0 as the pc is always aligned to a 2 byte boundary"
The rules are not clearly writen. They would want to be rewriiten for clarity.
- If the refence address is not valid then the current pc is recorded as
  the reference address and a trace message with a F-ADDR is sent.
  - When a message with F-ADDR is received then that address is recorded as the
    reference address.
- The value to transmit in U-ADDR is computed as the value resulting from XOR of
  the pc with the current reference address. This value now is the new reference
  address.
  - The bit 0 of this value is discarded and not transmitted.
  - The high order XLEN-1:Y bits are not transmitted if XLEN-1:Y, where Y
    ranges from XLEN-1 to 1, if they are all 0.
Section 8.2 - the sign extension of the U-ADDR must be done before XOR with
the reference address should be moved into normative text from the informative
text box. The rest of the warning text is redundant and can be removed.
Section 8.2 - the rules for forming U-ADDR or F-ADDR need to be normatively
defined. The section defines ruls for decoder but not for the encoder. There
is hint of it in the information box. That text states "Trace encoder must
implement a MSB extension" - the encoder needs to implement a detetction of a
run of 1 bits and not extension. There are unstated rules about sending at
least 1 bit in the run of 1 bits which are not described normatively.
Section 8.2.1 - there are too many examples for the simple concept. This section could be shortened.
Section 8.2.2 - the use of "MMU enabled" is misleading and should be replaced
with "when virtual memory system is enabled"
Section 8.2.2 - When trace is enabled for VS/VU mode, and Sv57x4 mode is actve
at G-stage and first stage is Bare, the pc is a GPA - to add to the list of 3
addresses - and can be 59 bits wide.
Section 8.2.2 - "Any illegal address.." the text in parenthesis may be removed.
Section 8.3 - reword "instead each taken...is adding a single bit as LSB..."
Section 8.3 - the description should not say HIST field since HIST is a
message field and is not the history accumulator. Further the description is
missing that this accumulator is a shift register where 1 or 0 are shifted
in from the LSB.
"starting from MSB bit (the one before stop-bit = 1)" should be "one after"
A better wording may be to describe it as a shift register for the decoder
as well.
Section 8.3 - the rule for generating the HIST+HREPEAT is not very clearly
defined. It should state when the stop bit is at MSB if the value is
identical to previously transmitted HIST then increment HREPEAT else send a
message with HIST + HREPEAT and initialize HREPEAT to 0.
Last para of section 8.3 seems redundant and not needed for decoders.
Section 8.3 The motivation for N-trace specification to limit HIST to 32-bit is not clear. If there is no functional
reason then it should be for the impleemntation to decide the cost. This limit should be removed.
15.Section 8.3 - "See Repeated..." - last senetnce - should be together with the text before the informational box.
Section 8.4 - the rule for increment of I-CNT can be stated more simply,
accurately, and a forward compatible manner as:
"Every retired instruction MUST increment I-CNT by its ILEN divided by 16.
The first sub-bullet can be removed - its redundant beyond stating "retired
instruction" as all retired instructions change PC.
The next two bullets "An exception or interrupt..." should be replaced with
"Exceptions and interrupts do not update I-CNT" Specifically the third
bullet is wrong as the I-CNT increment is a side effect of the instruction
retiring and not the act there was a interrupt between that instruction and
next. This statement should be a main bullet.
The last bullet can be removed with the wording as above.
Also the statement "are key rules" implies there are more rules?
Section 8.4.1 is a long example about I-CNT. This section should be written
to state the specifications normatively (if any) and the examples (if
required) non-normatively. Specifically this section does not add any new
value to the specification and can just be removed.
Again, maybe these belong in an Examples appendix at the end?
Section 8.4.1 - "Plain linear" is not an Priv. or Unpriv. term.
Section 8.4.2 - is not adding anything new to the I-CNt specification.
The rules for I-CNT increment and reset are agnostic to whether the
trace operates in HTM or BTM mode. This section can also be removed.
Section 8.4.2 - refers to "above code" but there is no code in this section
and lots of code in previous sections.
Too hard to correlate this with the scenarios in the prior section. Would be
better to show the dynamic execution flow, rather than the static code.
Section 8.4.2 - The non-normative note is repeated multiple times and adds
no value.
The entirety of section 8.4 including the 3 subsections can be replaced
with:
"The I-CNT is reset to 0 when trace starts (or restarts). Every retired
instruction increments I-CNT by its ILEN divided by 16. Traps caused by
exceptions and interrupts MUST not affect the I-CNT. The I-CNT is reset
to 0 when its value is transmitted in a trace message."
Section 8.4.4 - The wording is confusing and "(or when HIST buffer is empty)"
as a parenthis after "In BTM mode" implies HIST buffer has meaning in BTM
mode.
"may be reported" wrongly implies that it is optional to report overflow
of I-CNT.

It is not clear why ResourceFull cannot be used when HIST buffer is not
empty. Synchronizing the I-CNT should not require synchronizing the
HIST as well. The HIST can be sent when either it overflows or there is
message that inlcludes HIST that gets generated.

The statement "First choice... - second choice (SYNC=4).."
contradicts the rules of the two bullets. The first bullet states that
when HIST buffer is empty or when in BTM mode, the ResourceFull message
should be generated. But then this sub-bullet states that it is
optional and even in BTM mode a synchronization message can be generated.

The term "Synchronization message" is confusing. It should be specified
which precise *Sync message should be generated.

Suggested rewording:
"
An overflow of I-CNT must be reported to the decoder using one of the
following methods:
1. Using a ResourceFull message with RCODE=0. This method must be used
  when operating in BTM mode. This method may be used when operating in
  HTM mode if the HIST buffer is empty.
2. Using a IndirectBranchHistSync message. This method must be used
  if operating in HTM mode and the HIST buffer is not empty.
  "

The example, by including the c.ebreak muddles the example. That case is
not related to any kind of I-CNT overflow and is just confusing. Should
remove that from the example to keep the example crisp.

Could be Move this to examples in appendix

Section 8.4.4 - Reword "Overflown I-CNT=8 decodes" as decodes is
ambiguous.
Section 8.4.4 - the last paragraph implies periodic sync is mandatory.
The trace controls imply that periodic sync is optional. SYNC is needed for
various other reasons than periodic sync. This paragraph may read that
SYNC itself is not needed if periodic sync is not implemented. This paragraph
is not adding any further value and can be removed since it is misleading.
Section 8.5 - the title should be changed as there is no message called
synchronization. Perhaps "Synchronizing messages" is appropriate.
Section 8.5 - The first sentence should not be bulleted.
Section 8.5 - starting a section with an informational note is okay but
unusual.
Section 8.5 - "Allows the trade" -> "Allows the trace"
Section 8.5 - "Synchronization messages provide SYNC code" -> "Synchronizing
messages are messages with a SYNC field. The SYNC field identifies the reason for
synchronization and such messages include the F-ADDR (full address) field
to synchronize the PC with the PC observed by the encoder"
Table 25 - the header "Encoder Reset" should be first explained. Suggest
adding a few sentences before the table.
Table 25 - Fix the width of the columns
Table 25 - value 0 - "marker of external trigger input" - should this be
reworded as "marker to indicate that trace was enabled due to triggering a
external trigger"? Can external trigger be used to restart a trace that was
disabled? Which input indicates this in trace ingress port? Could we add
the name of the encoder input signal that triggers this? Which message
should be used to carry this SYNC field?
Table 25 - value 1 - Should this always be reset vector? What if trace was
activated only for S-mode. If a reset occured would trace then start tracing
M-mode PCs? Since a trace encoder capable of doing this must be in a
separate reset domain than the hart, should we break this into two - one
where there is a "stop" indicated due to reset assertion. And then if trace
starts there is a "start". So it seems appropriate to use "Entry to Reset"
here and on exit from reset when the trace is qualified to use Trace Enable?
Which message should be used to carry this?
Table 25 - value 2 - which message should be used to carry this sync.
"Clarify what is meant by "wrapped around".
Table 25 - value 3 - add an explicit section reference.
For each SYNC field encoding in Table 25 clarify which message is used to
carry them.
Table 25 - Trace Enable - the text indicates it shold be used only when
trace is re-enabled after being disabled for a gap. This does not match
What should be used when trace is enabled for first time?
Table 25 - Trace Event - should be renamed to more descriptive "
Debug watchpoint triggered". How is this different from external trace
trigger and which input to encoder should be used to determine this?
"with action=4" does this refer to debug spec? If so should provide a
reference. The "encoder state is not reset' is redundant as there is a
explicit column for this.
Table 25 - FIFO overrun - need a definition for FIFO overrun.
Table 25 - Exit from power down - how is it different than exit from reset?
Same questions about privilege mode filtering as for exit from reset. Is
this based on halted input? If not please clarify which input causes this.
Table 25 - code 10-13 - SHould not say Yes for endoder reset - this should
depend on whatever is future definition of these. Should be blank. In
description state "Reserved for future standard use"
Table 25 - code 14-15 - SHould not say Yes for endoder reset - this should
depend on how vendors specify this. In description update to say "Designated
for custom use"
Should resolve the open question about predioci sync usage and debugging.
Clarify what "Decorders should report" means.
Section 8.5 - clarify difference between full and partial TSTAMP. Or was it
meant to be absolute?
Section 8.5 - the informational note about "most synch. fully reset" - this
is not required as the table explicitly states which are and which are not.
Table 26 - please clarify why back to back return/jump/exception or
exception following interrupt are considered as corner cases.
Table 26 - "start of hart" - please clarify this term. If the interrupt is
pending why should it be signaled as a taken interrupt?
Table 26 - Why is excecption at first instruction traced a corner case?
Table 26 - "hart stops" - is that halted or reset? Why should a halt signal
entry to debug mode. Should the I-CNT and HIST not be the value active at
the time the hart stopped?
Section 8.7 - "relative to recently reported" - should it be any recently
reported or should it be fixed as the "last reported"?
Section 8.7 "ALL synchronization messages (which include full address)"
implies that only the synch. messages that include a full address must have
TSTAMP. However all such messages are required to have a full address. So
why is this qualification needed?
Section 8.7 - "Otherwise some sections..." that justification seems
incorrect as all sections are timestamped. The appropriate justification is
that sync. messages may be used to start decoding trace without having
observed any of the previous messages and hence carry an absolute timestamp
Section 8.7 - Is it allowed to omit TSTAMP in a message if the message defines
a TSTAMP field and trTsEnable is 1. The second bullet seems to imply that it
is okay to ignore the configruation. This also conflicts with second
paragraph that states that non sync. messages are required to incldue a
relative timetsamp.
Section 8.7 - it has been established earlier in Chapter 3 that maximum field
size is 64 bits. Why is the bullet 3 specially called out?
Section 8.7 - replace "code correlation" to "trace correlation"
Section 8.7 - reword 5th bullet
Section 8.7 - the first sub-bullet of 5th bullet talks about "distance
between distant events" - this is completely unclear what it means and
what is meant by "reported in a consistent way"
Section 8.7 - The 5th bullet should be reworded to state timestamp is
from the time the itype was presented to the encoder input. The statement
"when exception or interrupt was observed" does not make it clear as to
was observed by who. It should be when it was observed by encoder and not
when it was observed by the hart.
Section 8.7 - Rephrase - last main bullet. Don't need 3 bullets for this.
Section 8.2 - title - "Virtual Addresses Optimization" - consider removing plural
consider a different more descriptive name
"The following additional rules are used (when trTeInstExtendAddrMSB control bit is implemented
and set):" - remove parenthesis

Answer 1 · 2024-03-09T00:42:35.000Z

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