Feedback on chapter 11 - RISC-V N-Trace (Nexus-based Trace) Specification
ved-rivos opened this issue · 1 comments
-
"To decode an N-trace encoded stream of messages" - Decoding of the messages
does not require the listed 3 information. This should be reworded to
"To reconstruct the program control flow using the N-trace encoded stream of
messages" -
"Size of each instruction (16-bit or 32-bit)." drop text in parenthesis.
-
Second bullet - the decoder does not have access to itype signal on ingress
port. This bullet should be reworded to be written with respect to decoder. -
Third bullet - instead of stating "direct uncondition and direct cond" reword
to state "for inferable obtain the offset from the image" This will then
include the sequential jump optimizaztion - the current text precludes that
by saying "offset encoded in an opcode" -
"At the beginning of trace" should be rewritten to "Start decoding from a
synchronizing message. The synch. message provides the complete PC in the
F-ADDR field. Transfers relative to this PC may then be inferred using
subsequent messages till a new PC is transmitted in a subsequent sync.
message" -
"full PC is dropped periodically" - why should the decoder drop sync.
messages periodically? -
Section 11.1 - "complete PC flow" - complete seem overarching. COuld
reword to "reconstruct control flow of the program". "is very simple"
can be dropped to make the specification formal. -
Section 11.1 - Handle HIST ..(and not 0x1) - please expand/reword
"not 0x1" -
Section 11.1 - "analyze code from current PC" - "disassemble from the
last PC". "PC will be after last direct conditional" -> "PC will be of
the instruction executed after the last conditional branch" -
Section 11.1 - "each encountered instruction should subtract" ->
"For each instruction subract INST_LEN/16" - this will keep it straight
for future and for custom instructions that may be 48 or 64 bit or wider. -
Section 11.1 - Handle I-CNT - third sub-bullet - I-CNt being 0 does
not mean an event always. In case of ResourceFull due to I-CNT overflow
it is just the new PC. Suggest rewording as this main bullet as- First sub-bullet - "Dissaamble instructions starting at last known PC.
For each disassembled instruction, update PC based on the type of
instruction and decrementing I-CNT by ILEN/16, till ICNT becomes 0."
- First sub-bullet - "Dissaamble instructions starting at last known PC.
-
Section 11.1 - there seems to be priority between HIST and I-CNT? SO
HIST should be applied first and then I-CNT? This should be stated. -
Section 11.1 - missing the handling for HREPEAT and BCNT
-
Section 11.1 - information note - the "inferable unconditional jump"
is defined before - suggest not repeating -
Section 11.1 - information note - Encode under different names is ambigous
and all of this has been covered before. Repetitions should be removed. -
Section 11.3 - "(ELF files)" -> "(e.g., ELF files)"
-
Section 11.3 - Suggest rewording this section to be more precise on
the guidelines on use of Ownership message. What are the expectations
on the operating systems and hypervisors and the decoders. -
Section 11.3 - The informative text is not very useful and should be removed.
-
Section 11.3 - heading - Remove parenthetical, can refer to Linux in the text
-
Section 11.4 - in informative note please clarify what "compressing
execution flow" means. ALso please clarify how this comment relates
this section. If this is not specific to N-trace is there a reference
that can be provided to other trace systems that have solved thsi problem.
However, The informative text is not very useful and should be removed. -
Section 11.1 - Is there a reference decoder we can point to?
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