search

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
44 stars 32 forks source link

Feedback on RISC-V Trace Control Interface Specification - chapter 7 #54

Closed ved-rivos closed 6 months ago

ved-rivos commented 7 months ago

  1. trRamStopOnWrap - Its unclear from the specification what happens to the trace packets when storing is disabled. When storing trace is disabled are trace packets from upstream components dropped or does the trace RAM stop accepting packets and thereby cause a FIFO full leading to a stall/drop at the trace encoder?

  2. trRamAsyncFreq - the name of this field does not match with the description of this field.

  3. The guidance to not use trRamWRpHigh/trRamRPHigh in SMEM mode seems misplaced. While a trace buffer larger than 4 GiB may not be required, the trace buffer need not necessarily be placed in memory below 4 GiB.

  4. Table 47 - explain parameters M and N. For SMEM fixed is the guidance to set limit to trRamStart + 2^N - A correct? The 2^N seems to imply, on reading first column, the alignment such as 64 bytes or 128 bytes. The limit seems impractically small in that case. For SRAM mode, should limit be (max size - 4) instead of (max size); as suggested by the note following the Table 47 that A should be 4.

  5. Not being able to write the trace RAM - in SRAM mode - may lead to leakage of trace data among two traced entities that are mutually distrusting as the trace tool cannot clean up. Has the TG discussed the security model around this aspect?

mipsrobert commented 6 months ago

Here are done changes (see above for commit ID):

  1. DONE (updated description): Any stopped sink must stop accepting any input. Encoder may see an overflow or it may stall the hart.
  2. DONE (added word to describe it): Async means alignment synchronization.
  3. DONE (removed).
  4. DONE: These just saying that addresses and sizes should be power of 2. Fixed one (confusing) usage.
  5. NOT DONE: Security is not part of the spec. It may be revised if we ratify ‘secure debug’ spec. If someone wants secure way, they should implement writing (as it is optional). There are also other ways, better than writing all trace with 0-s to hide old trace.
ved-rivos commented 6 months ago

Table 47 - trRamLimit for SRAM mode is specified as hard coded to 2^M. It should be (2^M - A) as the bits 1:0 of trRamLimit are always 0.