Move nmi from etrigger to itrigger.

[Tim Newsome (timsifive)]

Per mailing list discussion subject:"etrigger.nmi".

[Paul Donahue (pdonahue-ventana)]

And tdata3? Or don't say "if the trigger is enabled for the current mode" and then it's implied that normal filtering applies. (Or is your goal to highlight that there was a change from the draft version?)

Also, something I mentioned in my email was that there can be multiple flavors of NMI and that tdata2 filtering can be useful (an argument that gfavor pointed out to me). If tdata2 is applied then I think that this should be explicitly called out because it might not be obvious that NMI support means that tdata2[0] must be implemented. Because mie[0] is architecturally hardwired to 0, supporting nonzero in that tdata2 bit would otherwise not be necessary and people may miss this additional requirement when supporting NMI.

Maybe something like this (which is similar to how etrigger talks about mcause):

When set, non-maskable interrupts cause this trigger to fire [if the trigger is enabled in the current mode and tdata3]. The trigger may fire on up to XLEN of the Exception Code values in mcause (described in the Privileged Spec section on Non-Maskable Interrupts). Those causes are configured by writing the corresponding bit in tdata2. (E.g. to trap on NMI with mcause Exception Code of 0, the debugger sets bit 0 in tdata2.)

[Tim Newsome (timsifive)]

OK, I'll remove the "if enabled" clause.

As for different NMI sources, let's leave that for a separate PR.

[Tim Newsome (timsifive)]

I'm struggling with this. I don't want to simply remove the clause, because "When set, non-maskable interrupts cause this trigger to fire" can be read to mean the mode bits don't matter at all. If I mention tdata3, then it should really be mentioned for every trigger, which it currently is not. Does that need to be added? It feels obvious to me that if you implement textra, then it applies, and if you don't implement it then it's irrelevant.

Which leaves me with exactly the language that's in the current PR.

What exactly did you intend with the square brackets in your example?

[gfavor]

On Thu, Mar 17, 2022 at 10:41 AM Tim Newsome ***@***.***> wrote: When set, non-maskable interrupts cause this - trigger to fire, regardless of the values of \FcsrItriggerM, - \FcsrItriggerS, \FcsrItriggerU, \FcsrItriggerVs, and \FcsrItriggerVu. + trigger to fire if the trigger is enabled for the current mode. I'm struggling with this. I don't want to simply remove the clause, because "When set, non-maskable interrupts cause this trigger to fire" can be read to mean the mode bits don't matter at all. If I mention tdata3, then it should really be mentioned for every trigger, which it currently is not. Does that need to be added? It feels obvious to me that if you implement textra, then it applies, and if you don't implement it then it's irrelevant.

People who read arch specs precisely would take the current text to mean that trigger firing is dependent on the stated dependencies and nothing more. Which in the above text is grossly misleading and requires one to groq other parts of the specs and reliably conclude that tdata3 DOES represent another dependency even though it wasn't stated as such in the above text. Or, more properly, they should be questioning whether that really is the arch intention since if it is, then the arch spec should have specified a complete and not an incomplete dependency list. In other words, specifying a dependency list implies that it is complete (unless the text explicitly says that it is an incomplete list). Removing any mention of dependencies in the above text would be an even bigger disservice to spec readers. Which implies that the tdata3 dependency needs to be mentioned everywhere that it applies - which seems like a straightforward adding of the same (or similar) sentence in a handful of places. Lastly, as a small soapbox comment, "hiding the ball" in arch specs is not a good thing to do. Yes, some diligent readers will piece everything together and figure out the correct arch intent. But others won't. And someone who is referring to one part of the spec for the definition of, say, a certain type of trigger, will easily not realize that the stated dependencies represent an incomplete list (since they maybe read the whole spec 6-12 months prior and don't remember every little detail). So it would be good practice to add the extra sentence in the handful of needed places and avoid some readers misunderstanding the full arch intent. Greg Message ID: ***@***.***>

…

[Tim Newsome (timsifive)]

So, let's merge this change as-is, and I'll create a new change to mention tdata3 (aka textra) in every trigger where it applies.

[Paul Donahue (pdonahue-ventana)]

I'm OK with that.

I agree that mentioning tdata3/textra in more places would improve the spec. It is somewhat in the background today so it's easy to overlook that it plays a role in a lot of things.