Feedback on RISC-V Trace Control Interface Specification - Chapter 6

Question

Feedback on RISC-V Trace Control Interface Specification - Chapter 6

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

ved-rivos commented 9 months ago

If the WARL is not the norm and there are exceptions then those should be
listed explicitly along with guidance on how to discover such options.
Reset values should be carefully selected. Only fields that absolutely need
a predefined reset should have a reset default. Please scan through the
registers to isolate the subset of state that must be reset. Rest should be
initialized by software.
trTeActive - The description stating this is a enable is misleading and
should be reworded. Since this bit works identically for all components
its description should move to section 4.3 so it does not need to be
repeated.
If trTeActive is written to 1 when it was 0 and some other bits of the
register are also written to 1 in the same write, do the other writes
stick or they are discarded?
The acronymn TE is used for firt time. Expand here or add to glossary.
trTeActive. When trTeActive goes from 1->0, what is the state assumed
by the components registers? Does it retain its state or lose its state?
If trTeActive is written to 0 when trTeEnable is 1 what is the behavior?
General - for single bit bits instead of "1:" one could write as "When set to
1.."
trTeEnable - add that "stops generating trace messages and flushes...".
The bit can be set by writing to it states the obvious. Is it allowed to
write 1 to this along with writing 1 to other settings in this same register
or is the statement only about fields not in this register? Should also
specify the behavior of what happens if other fields are written when this
bit is 1 - it could be unspecified i.e. some implementations may discard the
write, others may act upon some or all of new configuration, etc.
trTeInstTracing - This bit should be read-only and not read-write as it
is a status bit reporting whether tracing is active. The description "
written from a trace tool" seems wrong to have a volatile hardware bit be
also sw writeable. What is the purpose of this status bit to be SW writeable?
"may be subject to additional..in some implementations" - the normative
specification should not make any assumptions about custom extensions.
This sentence should be removed.
trTeEmpty - implies that the TE has internal buffers. Is that the intent?
or is this referring to HIST and CNT fields that may not have been emitted.
What are the conditions in which trTeEmpty will read 0?
Bit field location - please use conventional "msb:lsb" notation.
Fix column width so bit fields do not wrap.
trTeInstMode - the use "Main" implies there are other modes. should be
renamed to "Instruction trace generateion modes"
trTeInstMode encoding 0 - "trace may stil emit some records"
Terminology wise "records" is used for first time. Why does
a disabled encoder still generate messages should be explained.
trTeInstMode - encoding 1-2, 4-5 should just be called Reserved for
future standard use. The parenthetical repetition of for Branch Trace
and Branch History Trace should be avoided.
trTeInstMode - why does this reset to SD value?
trTeControl - bits 8:7 - the description should say Reserved for future
standard extensions. Should not predict what the extensions may be.
trTeContext - the description should exclude scontext/mcontext. The
description should be in form of encoder input which is context and
context may be asid/vmid for example or even hcontext. The description
should be restricted to avoid repeating specification - e.g., when Ownership
messages are generated, etc. The description "Send ownership.." implies
setting this bit will cause the sending. The description should be
"Enable generation of Ownership messages defined by RISC-V N-trace
specification"
trTeInstTrigEnable - The specification is indirect. The text also implies
that only the debug module may be used to do the triggering. Suggest
rewriting this to indicate - "Enable the use of trigger sideband signal
to start and stop instruction trace."
trTeInstStallOrOverflow - "by hardwre when an Overflow message is generated
or when the TE requests a hart stall" - implies that the Overflow message
is generated by some hardware other than the TE. The comment about reset
can be removed as there is a column for that - see also comment about
eliminating resets - this one would qualify as one that should have a reset.
The "Overflow message" is not explained anywhere. The N-trace specification
does not have a definition for such a message - nor does E-trace. This
likely implies a synchronization message generated following a fifo
overflow? It is not clear why this bit should get set once on stall and once
when a synchronization message is generated. For example, if at time T0
the bit was set due to a stall being asserted and SW cleared it at time T1
but the stall conditions persists and then at time T2 the stall condition
goes away leading to a synchronization message will this bit get set again?
Should also clarify that this bit only indicates that a stall has been
asserted since the last time this bit read a value of 0. This bit being 1
does not imply that the hart is currently stalled.
Question: What is behavior for data trace? Is a stall asserted? Can data
trace be active without instruction trace being also active? If so what
is synchronization for data trace?
trTeInstStallEna - do not need to repeat the synchronization message
following unstall that has been specified already.
trTeInhibitSrc - Suggest rewording as "When set to 1, the messages
generated by the trace encoder include a message source field if the
source width held in trTeSrcBits is not 0.
"Disable source.." -> "Disable inclusion of"
What is rationale to make this a inhibit control and not an enable control?
trTeInstSyncMode - "Select periodic.." -> "Select the periodic.."
Since 16-bit is implied by half-word, the parenthesized text can be removed.
Reword "Once the periodic counter is reached" to clarify what.
trTeFormat - "N-trace messages with..." -> Format defined by RISC-V N-trace
specification". The "with 6..." can be removed.
trTeProtocolMajor - "As specified by specification governing trTeFormat"
Table 11 - use period consistently. Follow structure "When set," consistently
trTeInstNoAddrDiff - instead of stating negative and then positive in
parenthesis could reword to state one way "When set, trace messages always
carry a full address"
trTeInstNoTrapAddr - "do not sent" -> "do not include"
Where these options had been discussed in E-trace spec - should copy the text
identically from Table 2.2 for maximal consistency.
trTeInstImplicitReturnMode - the text for encoding 0 is not clear and
should be rewritten. Why is this field R and not WARL.
encoding 1 - why support something that is not recommended to implement?
encoding 2 and 3 - need better descriptions. The logic cost etc. can be left
out of the specification - this has been discussed in the trace specs
themselves extensively and does not need to be repeated in the control spec.
trTeInstEnAllJumps - specify polarity for each action in "... or add..."
The trailing text is covered by the rtace spec and need not be repeated
here.
trTeInstExtendAddrMSB - it would be good if the E-trace and N-trace used
identical name for this optimization. That way the control can refer to the
optimization and not need to say more. The text in parenthesis should be
removed as that is covered by Priv. The last sentence should be removed.
trTeSrcID - reword the description.
trTeSrcBits - unclear why two controls are needed - The effect of setting
trTeSrcBits to 0 is same as setting trTeInhibitSrc. An implementation that
supports only 8-bit src-id may make this field WARL and only support 8 and
0 as the two configurations.
trTeInstFilters - this description should be written to be more clear about
what filters are being discussed and it means for `n' to match. If required
a link back to relevant trace specfication document could be provided.
trTeDataTRacing - this bit should also be a R as it provides status
information. Its unclear why the description notes "from a trace tool"
trTeDataStallOrOverflow - same comment as for corresponding instruction
trace control.
trTeDataStallEna - should clarify that the data trace messages are dropped
when set to 0 and data trace messages cannot be sent.
trTeDataDropEna - how does this control interact with trTeDataStallEna.
Will this bit override that control setting if the trTeDataSTallEna requires
hart stall? Will data trace be dropped if instruction trace is configured
to stall hart instead of dropping instruction trace?
trTeDataNoValue and trTeDataNoAddr - can these be set together. Is there
any value in data trace with trTeDataNoAddr=1 and trTeDataNoValue=0. The
other combination - trTeDataNoAdddr=0 and trTeDataNoValue=1 - seems useful.
Assume (trTeDataNoAddr=1 and trTeDataNoVaLue=1) is equivalent to disabling
data trace.
trTeDataFilters - same comment as trTeInstFilters.
Section 6.1 - "Internal System" - reword the sentence.
Section 6.1 - should put some ground rules on the timestamps. Is the
frequency of the timestamps required to be constant? If so then that should
qualify the clocks used - for example, if a core or a bus implements DVFS -
then is it acceptable to use that clock for timestamping? Is there any
requirement or guidance on the timestamp resolution.
Section 6.1 - its not clear that "internal system" and "internal core" need
be differentiated. The distinction made between accepting a timestamp from
"External" is also not clear. There needs to be some discussion on why these
four need to be exposed to the trace architecture and what logic would be
used to select among them. For example, when should a trace tool decide it
should use the shared vs. core vs. internal - what is the upside and what is
the downside of each choice.
Section 6.1 - "Shared" is this to be selected for the source and the
receivers that share the timestamp should be conifgured as "External"? If so
that should be clarified.
Section 6.1 - "The width of the timestamp..." - the text under parenthesis -
is quite obvious that a 40 bity counter overflows in 4.7 minutes at 1 GHz.
But is not clear what it is trying to imply - is it stating that a overflow
in 4.7 is not acceptable or is sufficient for all implementations?
"An Internal Timestamp Unit may include a prescaler divider" -> clarify this
is a clock divider.
Section 6.1 - "In a system with an Internal core timestamp counter..." The
last sentence is redundant - being optional implies it may or may not be
implemented. The first sentence shoudl be rewritten more formally.
trTsActive - what happens when this bit goes from 0->1 or 1->0. When tracing
is active can this bit be toggled?
Same question as trTsActive for trTsCount and trTsReset. Also what is the
behavior of these bits when trTsType is not 2 or 3.
Since trTsCOunt/trTsReset are only applicable for when trTsType can be
selected as as Internal, should these bits not be WARL?
trTsType - since this is a configuration - should this have been named
trTsMode and not trTsType whihc would imply a reporting facility.
trTsPrescale - its not clarified what n is . Since there are only 4
settings it may simple to just state 0: scale by 1, 1: scale by 4 ...
trTsEnable - Why is it called "Global Enable". Is there another local
enable for the encoder? If so then it should be pointed out.
Table 18 - Action (From debug Spec) -> mcontrol.action
Table 18 - for row 0 and 1 - just reference RISC-V Debug specification.
Table 18 - Action 2/3 - should also be qualified by the trace being enabled
i.e. trace has to be enabled for this action to take effect.
Table 18 - Action 4 - clarify which packet. "If trace is not active...
it should be ignored" - should be rewritten as observable behavior than
spec for hardware.
trTeTrigExtInAction0 - What is motivation to define input triggers as
encoded but output triggers as one-hot.
trTeTrigExtInAction0 - The text can be rewritten for clarifty. a) in
addition to trTeInstTrigEnable, there are other qualifiers such as
trTeEnable which are not included in the description. Is that intentional?
"it will start instruction tracing" - is that a statement about the
TE or the hart? The parenthetical text can be removed as that is a status
indication that tracing is in progress.
trTeTrigExtInAction0 - for trace-notify action should define which
packets are generated for E-trace and which for N-trace. Should also
clarify if this packet can be dropped if at the same instance a
synchronizing packet was also being generated by itself.
Section 6.3 - "And if a match bit in the trTeFilteriControl ...
..the corresponding register ... must have a valid value." - It is not
clear why if a match bit can be set, the corresponding register must
always have a valid value. This sentence may be more appropriate to
say that the corresponding register must exist? But then that statement
is superflous. Alternatively, this sentence could be updated to state
before a match bit in trTeFilteriControl is set, the corresponding register
must be setup with a valid value?
Section 6.3 - "Each of the mentioned ...
so a limited range can be matched with a single comparator unit if needed."
It is not clear what is implied by "so a limited range can be matched."
trTeFilteriMatchChoiceEcause - this is specified as a 32-bit register.
Exceptions cause values in RISC-V can be 64 values - this register should be
updated to support upto 64 exception causes.
Table 24 - trTeFilterMatchValueInterrupt - description is inaccurate
as this is used to match itype and not interrupt level codes.
Table 24 - trTeFilterEnable - what is the use case for an overall filter
enable. It would seem like enabling/disabling would require a single write
to this register and so the motivation to have this bit is not clear.
Suggest removing this as it does not seem to serve a valuable purpose.
trTeFilterMatchValueImpdef - missing parenthesis in description.
trTeFilterMatchMaskImpdef - missing parenthesis in description.
trTeFilterMatch?Impdef - In description indicate "impdef" refers to an
implementation defined set of hart to encoder input signals.
trTeCompPFunction - "less than to" -> "less than". "Greater than to" ->
"Greater than".
trTeCompPFunction - for arithmetic clarify whether the function is a
signed or a unsigned comparison. Also please add clarification that the
number of bits of the trTeCompPMatch used for comparison are the width
of the signals selected by trTeCompPInput.
trTeCompSFunction - instead of repeating the description - point to
trTeCompPFunction
trTeCompMatchMode - encoding 2 indicates - Either primary or secondary
result does not match - but the equation suggested !(P&&S) would match
if neither match - is that expected?
trTeCompPNotify - the parenthetical text should be made expelicit that
this control only applies when iaddr is the input.
trTeCompPNotify - "Final instruction in a block" - every final
instruction of a block generates a packet. Is this an additional packet?
What happens if the final instruction of that block is not reached for
example due to a trap that occurs before that? What happens if a
trigger condition turns off trace before the final instruction of the block
is reached?

Answer 1 · 2024-03-30T18:03:46.000Z

All notes to Control PDF handled in 1.0.0_rc20.