spec v0.5 ch 1 - ch 3 feedback

[bcstrongx]

By chapter.

3.1

• By default, the extension forbids the following debug operation in The debug operations affected by Zedsec extension unless they are explicitly granted to the external debugger. Linking to the text that immediately follows, including a capitalized word in the middle of the sentence, seems odd. Perhaps it would be more clear to say:

By default, the extension forbids the following debug operations unless they are explicitly granted to the external debugger.
• Entering Debug Mode
• Executing Program buffer
• Serving abstract commands (Access Register, Quick Access, Access Memory)

3.1.2

• I think you want "a" instead of "the" here: For the homogeneous computing system. Same for 3.2.1.
• triggers configured to enter Debug Mode will neither fire nor match in machine mode. is this necessary? If medbgen[i]=0, then debug mode can't access the trigger registers (since that requires M-mode privilege), so an external debugger couldn't setup a trigger to enter debug mode. With this extension I guess M-mode can now set dmode=1 on behalf of the S-mode debugger, but couldn't we rely on M-mode to keep tdata1.m=0 for such triggers, ensuring they never match/fire in M-mode? Or perhaps the HW could even force that, such that tdata1.m = wrval.m & ~(wrval.dmode & ~medbgen).

3.1.3

• nit, but I'd say "Sub-Machine Mode" rather than "Submachine Mode". Seems more clear to me that we're talking about modes below (less privileged than) M-mode. Maybe because submachine is an English word, e.g. submachine gun. After the title line, you could refer to sub-M-mode.
• Putting "and mdbgen[i]" at the end of this sentence is confusing: The submachine mode debug of hart i is determined by the sdedbgalw field of CSR msdcfg within hart i and mdbgen[i]. I recommend The submachine mode debug of hart i is determined by both mdbgen[i] and the sdedbgalw field of CSR msdcfg within hart i.
• The register access without halting is dropped. Is this referring to abstract commands? Would they be dropped, or cmderr=6?

3.2

• sec_inhibit seems a better name than sec_check
• the combined state of [sec_check, halted] as 0b11 remains unutilized If mtrcen=0 but medbgen=1, then we could enter debug mode from M-mode, and hence sec_check=halted=1. Unless you're intending to clear sec_check on entry to debug mode, but if so that should be explicitly stated.

3.3.2

• "External trigger" is an overloaded term in the debug spec, since both Sdtrig and the DM have different definitions. Probably would be wise to title 3.3 Sdtrig to make sure it is clear which type of external trigger is referred to here.

3.4

• I'm not sure why 3.4 is a separate chapter, why not address the CSR changes in sections 3.1, 3.2, and 3.3?
• Shouldn't Table 4 only care about M-mode vs not M-mode (aka sub-M)? To use debug mode at all requires that the debugger has at least supervisor debug access, which should mean any bits associated with S, U, VS, or VU modes are accessible.

3.4.2

• Table 5: Isn't "debug access privilege >= Sub-M" the same as no change? To get into debug mode at least Sub-M debug access privilege is required.
• Table 6: same comment as for Table 4 above
• Table 7: I think merging this into 3.3.2 would make this more clear. Also, the table caption has a typo ("sselect"), and the table column widths should be adjusted.

[AoteJin]

Thanks Beeman for the insightful feedback!

3.1
The list link is removed in upstream.

3.1.2
"tdata1.m = wrval.m & ~(wrval.dmode & ~medbgen)" must be enforced by hardware (according to table 6). The reason for statement "triggers configured to enter Debug Mode will neither fire nor match in machine mode." is to cover the case that a trigger might be set when medbgen[i] is 1 and not cleared before the signal is flipped to 0. In this case, the trigger must not fire/match when the medbgen[i] is 0.

3.1.3

• "submachine mode" is changed to "supervisor domain" in PR - Update the wording and add change overview in intro #38 .
• Yes, it is referring to a special kind of abstract command. I am fine with cmderr=6, do you see any potential use case for the error?

3.2

• There is an issue in trace repo A vector of sideband signals to inform the trace output being stopped  riscv-trace-spec#98 to track the naming. I will update this spec if the naming is resolved in the issue.
• It is a good question. In your scenario, I think 0b10 is good enough. Because it is not important to indicate trace encoder that there is a new upcoming event that potentially stops the trace when it is already stopped by previous event.

3.4

• The CSR changes are grouped together in this subsection, because some updates are quite scattered e.g dpc,dscratch*. But I agree, it might not be the best practice. Will try to merge it with other subsection.
• Regarding table 4, table 5, actually the privilege is associated with the debug access privilege ( specified in dcsr.prv/dcsr.v) instead of medbgen[i] and sdedbgalw. The reason to do so is make it more compliant with privileged-based check and leave some room in case U-mode knob is added in the feature.

3.4.2
Regarding table 5, yes you are right. Defined in this way just in case U-mode knob is added.

I made updates according to your feedback in PR #38

[bcstrongx]

3.1.3

• Yes, it is referring to a special kind of abstract command. I am fine with cmderr=6, do you see any potential use case for the error?

Without cmderr=6 the user may assume the command was successful, if the last command left cmderr=0.

3.2

• It is a good question. In your scenario, I think 0b10 is good enough. Because it is not important to indicate trace encoder that there is a new upcoming event that potentially stops the trace when it is already stopped by previous event.

But since halted already has an established behavior, it may be better to go with 0b01. So define sec_check as 0 when halted=1.

3.4

• Regarding table 4, table 5, actually the privilege is associated with the debug access privilege ( specified in dcsr.prv/dcsr.v) instead of medbgen[i] and sdedbgalw. The reason to do so is make it more compliant with privileged-based check and leave some room in case U-mode knob is added in the feature.

I see. Is that necessary though? If medbgen=1 and a halt occurs in U-mode, why would we prevent the debugger from being able to write all the bits? dcsr is already a special case, it is only writable in debug mode today, no other priv mode check apply. So rather than introducing complicated priv mode checks per field, couldn't the fields be put into one of two buckets (requires medbgen=1 and doesn't require medbgen=1)?

btw, there is a typo in table 4. "ebeaks" -> "ebreaks"

All the other stuff sounds good.

[AoteJin]

I think you make sense for 3.1.3 and 3.4. Updated in PR #38

I suppose it is more appropriate to define of the behavior of vector [sec_check, halted] in trace repo, could we move the discussion over there?

[gokhankaplayan]

Are you only referring to command.cmdtype=1 (Quick Access) here for cmderr=6? If Yes, I am fine with the change.
If you are also referring to command.cmdtype=0/2 (Register and Memory Access), cmderr set to 4 (halt/resume). Can you please clarify that?

3.1.3

Yes, it is referring to a special kind of abstract command. I am fine with cmderr=6, do you see any potential use case for the error?

[AoteJin]

Actually I am referring to
"13. Registers can be accessed without halting. (Optional)"

In this case, it is not appropriate to set cmderr to 4.

[bcstrongx]

I don't see the changes for 3.1.3 and 3.4 in PR #38 ?

And yes, moving the sec_check discussion to the trace issue sounds good.

[bcstrongx]

Actually, I take it back, I think changes are needed in this spec for the encoder interface. The interface is changing to an encoded input (name TBD, based on the trace issue you linked above), so this spec should not refer to individual bit names. [sec_check, halted] is misleading, since the implementation actually needs to do something like:

traceinh = 0b00
if (halted) then
    traceinh = 0b01
elsif (sec_check) then
    traceinh = 0b10
endif

[AoteJin]

Hi @bcstrongx, the updates based on your feedback are included in PR #47 . Thanks a lot!
Regarding the trace signal definition, I will follow the naming accepted by trace spec when it is finalized. The solution in truth table looks pretty good, I prefer to reference the trace spec instead of defining it. Do you think the external debug spec need to definite it specifically?

[bcstrongx]

Great, I'll review the PR this week

[AoteJin]

Close as PR merged to main branch.