timsifive
commented
2 years ago Section 2.1.2: Each of those triggers can be one of 4 types
There are at least 5 trigger types: icount, itrigger, etrigger, mcontrol6, tmexttrigger. Maybe more if you count mcontrol and legacy SiFive triggers (which are both deprecated) and custom triggers (which are not). So something more vague like "several" is better than a fixed value.
Fixed.
Also in 2.1.2: It’s permissible for features to be implemented, but not in all combinations.
That sentence was confusing for me. Maybe something like "It's permissible for features to be implemented in some combinations but not other combinations."
"These options may be implemented in arbitrary combinations, which might not have a concise description."
In section 4.2 "Extending the Schema" it's not clear to me if step 4 involves official ratification of the proposed extension. Mainly, I want to know if step 5 only happens for ratified extensions and not for draft versions.
New text: "4. The task group treats this PR as part of their specification, and it will go through the regular approval/ratification process along with the specification. Once that process is complete, they update the PR indicating so."
Section 4.3 item 4:
Some INTEGERs, such as number of harts, might have a large range but are usually small numbers. Leave those without upper bound instead of using the absolute highest value they could have. Example: hartid INTEGER (0..MAX)
That example appears to violate its own suggestion by including an upper bound. Or is the idea that MAX is a variable instead of a fixed value? If so, that could be clearer (from the perspective of someone who doesn't know ASN.1).
Yeah, MAX is really a synonym for infinity here. I'll clarify that.
Section 6.1: The binary Configuration Structure is accessible by performing reads on the system bus.
System bus access is optional in the debug spec, so if it's not implemented then does this whole thing fall apart with respect to the external debugger?
I assume that the system bus is what harts access when they perform memory accesses, so the debugger can use the program buffer to access the CS.
Section 7 has a typo here: following the format descriped in the schema
Fixed.
A couple of places refer to the value of XLEN. XLEN means the size of integer registers in the current mode, though the current mode is not static and the CS is. When referring to the value of XLEN, does that mean DXLEN (which is fixed)? Does it mean MXLEN (which software might be able to change but presumably won't change dynamically)? Or does CS contain something like SLXEN (1..2) and UXLEN (1..2) if those values can be changed by software?
Good question. I suppose I'm subconsciously fighting the notion of XLEN being writable. Mostly XLEN is used as an example of a basic property of a hart. Is there a term for the largest value that XLEN might have? I'm going with "MXLEN at reset" for now.
Section 7.3 implies at the end that pointers are 8 bytes. Is this CS standard forward-compatible with RV128?
I don't understand what language implies that. My section 7.3 reads: 7.3. Conflicting Information It is possible to generate a CS with conflicting information, but such a CS is not valid, and must not be created or used. Conflicts occur when an element in a type describing a component has one value, and then later that same element in a different type describing that same component has another value. OPTIONAL elements can be missing in one description and be present in another. That is not a conflict.
As far as I know everything we've done should work in RV128.
The second paragraph in section 8.2 is missing a period at the end.
Fixed.
Does this support describing custom extensions, custom triggers, custom satp.MODE values, custom CSRs, etc.? If a particular custom extension is popular enough, debuggers would presumably support it and want to detect it.
There is support for totally custom information going into the CS. (See the Custom type in configuration-structure.asn.)
Custom triggers will need some thought. I don't know how much we can meaningfully say. I suppose we can enumerate other types that are supported.
I don't know anything about satp.MODE values. That should go into a (currently non-existent) privspec section.
Custom CSRs should I guess also go into a privspec section. We can enumerate which ones exist. Should we attempt to describe anything about them beyond that?
pdonahue-ventana
allenjbaum
I have several comments and questions which I'll lump into one issue for convenience.
There are at least 5 trigger types: icount, itrigger, etrigger, mcontrol6, tmexttrigger. Maybe more if you count mcontrol and legacy SiFive triggers (which are both deprecated) and custom triggers (which are not). So something more vague like "several" is better than a fixed value.
That sentence was confusing for me. Maybe something like "It's permissible for features to be implemented in some combinations but not other combinations."
In section 4.2 "Extending the Schema" it's not clear to me if step 4 involves official ratification of the proposed extension. Mainly, I want to know if step 5 only happens for ratified extensions and not for draft versions.
Section 4.3 item 4:
That example appears to violate its own suggestion by including an upper bound. Or is the idea that MAX is a variable instead of a fixed value? If so, that could be clearer (from the perspective of someone who doesn't know ASN.1).
System bus access is optional in the debug spec, so if it's not implemented then does this whole thing fall apart with respect to the external debugger?
Section 7 has a typo here:
A couple of places refer to the value of XLEN. XLEN means the size of integer registers in the current mode, though the current mode is not static and the CS is. When referring to the value of XLEN, does that mean DXLEN (which is fixed)? Does it mean MXLEN (which software might be able to change but presumably won't change dynamically)? Or does CS contain something like
SLXEN (1..2)andUXLEN (1..2)if those values can be changed by software?Section 7.3 implies at the end that pointers are 8 bytes. Is this CS standard forward-compatible with RV128?
The second paragraph in section 8.2 is missing a period at the end.
Does this support describing custom extensions, custom triggers, custom satp.MODE values, custom CSRs, etc.? If a particular custom extension is popular enough, debuggers would presumably support it and want to detect it.