Tighten up trigger specification.

[timsifive]

This contains a bunch of improvements that came out of an e-mail
discussion. The biggest one is that it specifies the priority of various
triggers compared to the exceptions already in the Privileged Spec.

[timsifive]

The priority table as rendered:

[billhuffman]

Execute looks good.

There's an entry in the middle where the description says "load/store/AMO address breakpoint" but the trigger says "dmcontrol data before load/store." Does the trigger entry intend to be "address before load/store"?

Assuming that, there's a data before load entry but not store. Is that what you meant? For implementations that cannot do a "before data" for either load or store, how will they proceed? Not allow the trigger to be written to those values?

And I'm still unsure why "after" is the lowest priority. I presume it's always taken with EPC pointing to a younger instruction than the one causing the trap. On that younger instruction, does it still want to be lowest priority?

[pdonahue-ventana]

A few initial comments. I may have more tomorrow.

[pdonahue-ventana]

But didn't you say above that the epc will be the next instruction? So it's not some indeterminate time, it's after this instruction and before the next.

[billhuffman]

If it's required to be the next instruction, some implementations will run several times slower when such a trigger is enabled.

[pdonahue-ventana]

I misinterpreted https://github.com/riscv/riscv-debug-spec/pull/478/files#diff-450f64c063fbaea18eda4faa7cfabfecR59 to mean that imprecise required mepc to be the instruction after the trigger rather than what it actually says: "the address of the next instruction to be executed."

[timsifive]

It's not required to be the next instruction, although that is preferred.

Is there anything I should change in response to this thread?

[pdonahue-ventana]

No. It was my mistake.

[ernie-sifive]

I think you mean mcontrol (trigger type 2) instead of dmcontrol. And should this table also include the itrigger/etrigger types?

[timsifive]

I do mean mcontrol. Thanks.

I decided not to include itrigger/etrigger because their description is already very clear when the trigger is taken. ("When the trigger fires, all CSRs are updated as defined by the Privileged Spec, and the requested action is taken just before the first instruction of the interrupt/exception handler is executed.") Furthermore interrupts aren't precise so don't belong in this table regardless. Including exceptions would clutter up the table a lot, and I don't think it would add clarity.

I'm happy to change things if that doesn't make sense.

[timsifive]

@billhuffman You're right about the missing entries. I've fixed that so now every possible mcontrol (thanks, Ernie) trigger is listed.

And I'm still unsure why "after" is the lowest priority. I presume it's always taken with EPC pointing to a younger instruction than the one causing the trap. On that younger instruction, does it still want to be lowest priority?

I've assumed the opposite: that EPC will point at the instruction causing the trigger to fire (assuming precise exceptions). This is the same way all other exceptions work. I've made this explicit in this change as well. You can see other changes besides just this table if you click the "Files changed" tab above.

[timsifive]

Here's what the table looks like now:

[billhuffman]

@timsifive The timing=before triggers should have EPC pointing to the instruction that caused the trigger. But I'm expecting the timing=after triggers will have their EPC pointing to at least the instruction after the one causing the trigger and, depending on the implementation, perhaps a number of instructions after. That's the reason for my reference to younger instructions.

In that context, then, I'm wondering why the timing=after priority is listed as lowest. I would assume that, on the later instruction that was trapping, a timing=after trap would be higher priority than the traps for that instruction. In the case where the timing=after trap happens on the instruction immediately after the one causing the trigger, I would expect the trigger trap to be higher priority even than an instruction address breakpoint and probably than an icount breakpoint.

[pdonahue-ventana]

This is simply a style thing but when I initially parse "mcontrol data before load" it seems to me that it's drawing a temporal relationship between data and load (and the data is before the load). I understand it in context but this could be confusing for newcomers.

How about using mcontrol followed by the order used in table 5.8? (load/store/execute followed by the select field followed by the timing field such as "mcontrol execute address before" and "mcontrol load data before") That's not perfect but the fact that the preposition is dangling indicates that it's not intended to be part of a prepositional phrase.

[timsifive]

Sounds reasonable. Changed.

[pdonahue-ventana]

In looking at table 5.8, I'm reminded that there are execute address+instruction breakpoints via chain=1.

I think that there needs to be a sentence that when triggers are chained, the priority is the lowest priority of the triggers in the chain. That means that execute address+data will be treated like execute data regardless of the order of the address and data triggers in the chain.

On a related note: Should there be a requirement that higher priority things come earlier in the chain? If trigger 0 is execute data chain=1 and trigger 1 is execute address chain=0, it seems weird to say that the execute data trigger (which comes after fetching the instruction) is preventing the execute address trigger (which comes before fetching the instruction) from matching. It would be more natural for something earlier in the process to prevent something later in the process from matching.

[timsifive]

Extra sentence makes sense.

I don't see a big advantage to forcing the debugger to set up the chain in a specific order. The way I imagine it, at each "stage" of executing an instruction the hardware checks any trigger conditions, and sets a hit bit (possibly internal) for each matching trigger. Then it looks at those hit bits and the chain bits and causes a trigger when all that looks OK. The order is irrelevant then.

[timsifive]

@billhuffman I agree with you on the priorities, but I'm still coming around to the idea that after triggers end up with DPC pointing at the next instruction. It makes a fair amount of sense, allowing more time to perform the comparison of data loaded from memory. But so far I'd always assumed it wasn't the case. Does anybody else want to weigh in on what DPC should contain when an after trigger fires?

[pdonahue-ventana]

@billhuffman I agree with you on the priorities, but I'm still coming around to the idea that after triggers end up with DPC pointing at the next instruction. It makes a fair amount of sense, allowing more time to perform the comparison of data loaded from memory. But so far I'd always assumed it wasn't the case. Does anybody else want to weigh in on what DPC should contain when an after trigger fires?

There are two possibilities:

1. In implementations with precise timing=after triggers, dpc points to the instruction that hit the trigger and in implementations with imprecise timing=after triggers, dpc points to the first instruction that didn't execute.
2. In all implementations, dpc points to the first instruction that didn't execute. In the precise case, this will be the instruction after the one that hit the trigger.

It seems that 2 is the right answer since I don't think that the debugger has any way of detecting precise/imprecise and figuring out whether to increment dpc before restarting (though I may be forgetting something).

You can either put pending timing=after triggers at the top of the priority list or you could omit them altogether and treat them like interrupts (which have similar mepc rules and are imprecise). Technically, all timing=after triggers would be imprecise but some implementations could decide to have a very strict bound on the imprecision (limiting it to the next instruction). That would be up to them.

[billhuffman]

@pdonahue-ventana If possibility #1 is true, then I don't see how timing=after triggers differ from timing=before triggers. The idea of timing=after, I think, is to have the instruction that caused the trigger complete so you can see its results and then stop.

I agree with possibility #2 for timing=after triggers. For many implementations, timing=after triggers will trap on (DPC points to) the instruction immediately following the one that triggered. For some implementations and some triggers, timing=after will trap on an instruction later than the one immediately following.

As for the priority, I think the pending timing=after triggers have to be at the top of the priority list. If they are not at the top of the priority list, will they ever be taken while the debugger is single-stepping through code? It seems like they will not be taken, if they are lower priority than icount, until the debugger stops single-stepping.

I think that, effectively, interrupts need to have a place in the priority list that's ahead even of timing=after trigger exceptions or else they also won't be taken when in single-step mode. And for implementations where a cycle or two of interrupt latency matters, they also need to be at the top to be taken as soon as possible.

[pdonahue-ventana]

@billhuffman

@pdonahue-ventana If possibility #1 is true, then I don't see how timing=after triggers differ from timing=before triggers. The idea of timing=after, I think, is to have the instruction that caused the trigger complete so you can see its results and then stop.

We're in agreement that timing=after means that the instruction that caused the trigger completes.
"The action for this trigger will be taken after the instruction that triggered it is executed." My assumption before yesterday was that dpc would point to the first instruction that hasn't executed so it could never point to the instruction that matched the trigger. I believe that's what you're saying but you should note that the proposal we're discussing has added precise timing=after triggers:
https://github.com/riscv/riscv-debug-spec/pull/478/files#diff-450f64c063fbaea18eda4faa7cfabfecR58

"If the trigger is precise, mepc must contain the address of the instruction that caused the trigger to fire. If the trigger is imprecise mepc will contain the address of the next instruction to be executed."

That may have always been the implicit assumption but I don't like that for the reasons I previously stated (basically because there's no way to determine whether it's precise or imprecise so the debugger can't know whether to return to dpc/mepc or to increment to the next instruction).

As for the priority, I think the pending timing=after triggers have to be at the top of the priority list. If they are not at the top of the priority list, will they ever be taken while the debugger is single-stepping through code? It seems like they will not be taken, if they are lower priority than icount, until the debugger stops single-stepping.

We're either going to halt/trap due to icount or due to the trigger, right? The reason we stop doesn't seem important to me since you're going to stop either way.

I think that, effectively, interrupts need to have a place in the priority list that's ahead even of timing=after trigger exceptions or else they also won't be taken when in single-step mode.

Interrupt prioritization is beyond the scope of what we can change here.

[pdonahue-ventana]

A coworker just pointed out that itrigger and etrigger aren't in the table. It seems that they're the highest priority thing when executing the first instruction of the handler (before the instruction fetch actually happens) so they belong at the top with icount. I think that itrigger and etrigger are mutually exclusive so those two are basically at the same priority.

As for their relative priority to icount, it seems like a don't-care to me. If the hit bit isn't implemented then there's no way to tell which one was taken so relative priority doesn't matter at all. If the hit bit is implemented and we hit itrigger (or etrigger) and icount at the same time, I propose that it should be allowable for either or both of the hit bits to be set (meaning that the implementation can choose whether itrigger/etrigger is higher priority, icount is higher priority, or they are at the same priority - the latter seemingly being preferred). Same for pending timing=after triggers (assuming the proposal by @billhuffman to make them high priority).

[billhuffman]

On 7/26/19 4:47 PM, Paul Donahue wrote: EXTERNAL MAIL @billhuffman<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_billhuffman&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=XlcY8yK_hqqX0cyJT5m2HZC3SVXUJOEZrarMh1w5O7k&e=> @pdonahue-ventana<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_pdonahue-2Dventana&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=yq5TxHHAmkrJE1xaSvvyX8CaYRGHou9AyTh5Cl7_evs&e=> If possibility #1<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_riscv_riscv-2Ddebug-2Dspec_pull_1&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=ueeLlnPL4jLreCpcZMOP-uoVDuldaEpSz8wInXXsgi0&e=> is true, then I don't see how timing=after triggers differ from timing=before triggers. The idea of timing=after, I think, is to have the instruction that caused the trigger complete so you can see its results and then stop. We're in agreement that timing=after means that the instruction that caused the trigger completes. "The action for this trigger will be taken after the instruction that triggered it is executed." My assumption before yesterday was that dpc would point to the first instruction that hasn't executed so it could never point to the instruction that matched the trigger. I believe that's what you're saying but you should note that the proposal we're discussing has added precise timing=after triggers: https://github.com/riscv/riscv-debug-spec/pull/478/files#diff-450f64c063fbaea18eda4faa7cfabfecR58<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_riscv_riscv-2Ddebug-2Dspec_pull_478_files-23diff-2D450f64c063fbaea18eda4faa7cfabfecR58&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=qpoeynqegrJ_vZ-3Gpx2Vt2RYOEMWne29t9R8eReY9U&e=> "If the trigger is precise, mepc must contain the address of the instruction that caused the trigger to fire. If the trigger is imprecise mepc will contain the address of the next instruction to be executed." This all seems fine, but it seems to imply that every timing=after trigger must produce an imprecise exception. That's OK from my point of view, but is it intended? That may have always been the implicit assumption but I don't like that for the reasons I previously stated (basically because there's no way to determine whether it's precise or imprecise so the debugger can't know whether to return to dpc/mepc or to increment to the next instruction). As for the priority, I think the pending timing=after triggers have to be at the top of the priority list. If they are not at the top of the priority list, will they ever be taken while the debugger is single-stepping through code? It seems like they will not be taken, if they are lower priority than icount, until the debugger stops single-stepping. We're either going to halt/trap due to icount or due to the trigger, right? The reason we stop doesn't seem important to me since you're going to stop either way. I see what you mean. But that does introduce a requirement that trap handlers need to know what other traps they have to check for. Worse, some traps can't be "checked for" because they don't leave a trace if they aren't the trap taken. Maybe this all works out, but it seems to me more straight forward to say imprecise traps are high priority. I think that, effectively, interrupts need to have a place in the priority list that's ahead even of timing=after trigger exceptions or else they also won't be taken when in single-step mode. Interrupt prioritization is beyond the scope of what we can change here. Sure. It was just a statement of what I thought the effective result for interrupts was. No intent to discuss interrupt priority specification. Bill — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_riscv_riscv-2Ddebug-2Dspec_pull_478-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAKXXKKEVTCSERFIHACJA2M3QBOEKHA5CNFSM4IG7OW42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD256M6Q-23issuecomment-2D515630714&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=sk6_0UcgctwZTuokTaDfYDI5acMb73LUMPvMzHNmrO8&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AKXXKKB2WSMBZ6BOSCR7WRTQBOEKHANCNFSM4IG7OW4Q&d=DwMCaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=AYJ4kbebphYpRw2lYDUDCk5w5Qa3-DR3bQnFjLVmM80&m=EgYefH8UWLuG8FpnNtd2CgZsgirytJXCJd0iHQZaCms&s=YJkZGTH6wMmylsDY4HvfmDHUW2HAnA9UP7a_F_zBArk&e=>.

[pdonahue-ventana]

@billhuffman

But that does introduce a requirement that trap handlers need to know what other traps they have to check for. Worse, some traps can't be "checked for" because they don't leave a trace if they aren't the trap taken.

Note that if the optional "hit" bit isn't supported, I think that all debug traps don't leave a trace, even if they are the trap taken. We get mcause=3 ("breakpoint") and the mepc and nothing else. That is probably enough to figure out precise traps since you can examine the instruction at mepc and figure out what it hit by knowing all the programmed triggers and examining:

• the mepc for execute address
• the instruction encoding for execute data
• the source register plus offset specified in the instruction encoding for load/store address
• the source register specified in the instruction encoding for store data
• the memory contents for load data
• whether the mepc is a trap vector for etrigger/itrigger
• just the fact that you stopped if you were using icount

But if you have imprecise timing=after breakpoints then I have no idea how a handler is supposed to know what's going on because it can't examine any of the above.

Maybe if you support imprecise timing=after then you have to support hit (either because the spec could add something to dictate that or because you come to the realization that to do otherwise would make for a less-than-compelling product).

(In the above you can replace "trap" with "halt" and "mepc" with "dpc" and "handler" with "external debugger" and I think it still all applies.)

[billhuffman]

@pdonahue-ventana You're saying there are more cases than I was thinking of where it's hard to find what happened. Hard to find what happened is bad enough, but when multiple things happen and there's no way to know that things past the first happened, it's even worse. In that case we don't even have the reason to hunt for what happened - because we think we know.

Seems to me something has to be done to ensure that we can tell what happened on the precise cases - and then, eventually, the imprecise cases as well....

[timsifive]

Thanks for keeping this discussion going. I've updated the PR:

• dpc is always to the next instruction that needs to be executed
• timing=after priority is bumped to the top, with a note that this is for a previous instruction
• Added itrigger/etrigger after all. (Initially I decided to omit it from this table, but now I think it does fit.)

[pdonahue-ventana]

I would find it clearer to say that the side effects are due to the memory access and to include a clear warning such as:

If this is combined with \FcsrMcontrolLoad then a memory access will be performed (including any side effects of performing such an access) even though the load will not update its destination register. Debuggers should consider this when setting such breakpoints on, for example, memory-mapped I/O addresses.

[timsifive]

Sounds good.

[pdonahue-ventana]

I don't think that there was any comment on my previous proposal:

As for their relative priority to icount, it seems like a don't-care to me. If the hit bit isn't implemented then there's no way to tell which one was taken so relative priority doesn't matter at all. If the hit bit is implemented and we hit itrigger (or etrigger) and icount at the same time, I propose that it should be allowable for either or both of the hit bits to be set (meaning that the implementation can choose whether itrigger/etrigger is higher priority, icount is higher priority, or they are at the same priority - the latter seemingly being preferred). Same for pending timing=after triggers (assuming the proposal by @billhuffman to make them high priority).

I'm OK with specifying the priority in some specific way but I just wonder if it's really necessary to disallow hit=1 on both an icount trigger and an itrigger trigger when you single step and get an interrupt.

It also seems unnatural to have etrigger and itrigger not next to each other (which would make them effectively the same priority since they're mutually exclusive). As written, if you single step and get a trap due to an exception, you get an etrigger (not icount). If you single step and get a trap due to an interrupt, you get icount (not itrigger).

[timsifive]

My reading of this table is that the horizontal lines (\hline in the source) go to the next priority level. So the lines you quoted are all at the same priority. I'll add a clarification. (I believe the original in the Privileged Spec has the same interpretation, although it doesn't explain it.) Since they're all at the same level, I ordered them alphabetically. I'm happy to change them, though.

I'm not sure what you mean by disallowing hit=1 on multiple triggers. AFAICT the language is a little ambiguous on when hit may be set when multiple triggers are hit at the same time. That's probably worth addressing once we've got this priority table in order.

[pdonahue-ventana]

If they're at the same priority then my hit=1 comment doesn't apply.

[timsifive]

Looks like people have stopped commenting, so I assume this is good to go. @ernie-sifive, can you give this an approval?