Clarify action=1 (enter Debug Mode) with dmode=0 #501
Conversation
Otherwise you might end up in a situation where regular software can control when to enter debug mode.
trigger.tex
Outdated
|
|
||
| This action is only legal when the trigger's \FcsrTdataOneDmode is 1. | ||
| Hardware should ignore triggers which have this action configured without | ||
| having \FcsrTdataOneDmode set. \\ |
There was a problem hiding this comment.
Would it be better to say that hardware must not accept a write to mcontrol with dmode=0 and action=1. rocket-chip forces action to 0 if dmode is being written with 0.
|
There are 4 possible scenarios (in what I think is the most complex to the
simplest HW implementation order):
- ignore the write and return an error code
- clear the action bit
- allow the write, but inhibit the trigger
- clear the output of the action bit, but not the action bit state
Note that because of the semantics of CSRRC and CSRRS, we may not be
actually writing the action bit if we just clear dmode, and not writing to
dmode bit if we are just setting action, so it is not the act of writing
that is important, but the final value of the bit
I'd prefer one of the last two, but could probably live with the rocket
implementation.
…On Thu, Nov 21, 2019 at 3:06 PM Ernie Edgar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In trigger.tex
<#501 (comment)>
:
> \RcsrDpc must contain the virtual address of the next instruction that must
- be executed to preserve the program flow. \\
+ be executed to preserve the program flow.
+
+ This action is only legal when the trigger's \FcsrTdataOneDmode is 1.
+ Hardware should ignore triggers which have this action configured without
+ having \FcsrTdataOneDmode set. \\
Would it be better to say that hardware must not accept a write to
mcontrol with dmode=0 and action=1. rocket-chip forces action to 0 if dmode
is being written with 0.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#501?email_source=notifications&email_token=AHPXVJTGF3Z2TAOHOIWMNODQU4H77A5CNFSM4JP2ENJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMS5NQQ#pullrequestreview-321246914>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJTRVPPWI4Z22OULX6TQU4H77ANCNFSM4JP2ENJQ>
.
|
|
Based on the above, the spec change is #3, but rocket implements #2
#1 doesn't work if there is no external debugger; you can still ignore the
write, but can't return an error code.
I'm thinking that most people are implementing the rocket implementation.
Putting on my compliance hat: That makes the combination of (dmode,action)
be a WARL field with the lone illegal setting of 01 being forced to 00. I
*think* the proposed compliance framework WARL semantics can deal with
that; there is a conditional flag that can be put on a field to select
between two formats (e.g. RW or ROZero). Otherwise, they are non-contiguous
bits and we can't treat them as a field, and it's nasty
#3 or #4 is easiest for compliance purposes.
On Thu, Nov 21, 2019 at 6:32 PM Allen Baum <allen.baum@esperantotech.com>
wrote:
… There are 4 possible scenarios (in what I think is the most complex to the
simplest HW implementation order):
- ignore the write and return an error code
- clear the action bit
- allow the write, but inhibit the trigger
- clear the output of the action bit, but not the action bit state
Note that because of the semantics of CSRRC and CSRRS, we may not be
actually writing the action bit if we just clear dmode, and not writing to
dmode bit if we are just setting action, so it is not the act of writing
that is important, but the final value of the bit
I'd prefer one of the last two, but could probably live with the rocket
implementation.
On Thu, Nov 21, 2019 at 3:06 PM Ernie Edgar ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In trigger.tex
> <#501 (comment)>
> :
>
> > \RcsrDpc must contain the virtual address of the next instruction that must
> - be executed to preserve the program flow. \\
> + be executed to preserve the program flow.
> +
> + This action is only legal when the trigger's \FcsrTdataOneDmode is 1.
> + Hardware should ignore triggers which have this action configured without
> + having \FcsrTdataOneDmode set. \\
>
> Would it be better to say that hardware must not accept a write to
> mcontrol with dmode=0 and action=1. rocket-chip forces action to 0 if dmode
> is being written with 0.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#501?email_source=notifications&email_token=AHPXVJTGF3Z2TAOHOIWMNODQU4H77A5CNFSM4JP2ENJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMS5NQQ#pullrequestreview-321246914>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHPXVJTRVPPWI4Z22OULX6TQU4H77ANCNFSM4JP2ENJQ>
> .
>
|
|
A debugger (or software) should never write an illegal value. Hardware may choose to protect itself in case it does. I have no strong feelings if that happens by changing the value as it's written to something legal, or by ignoring the illegal trigger. Should I just mention both, or just say something vague like "hardware should ensure not to enter Debug Mode through a trigger where dmode=0?" |
How does an error code get returned on a CSR write?
Note that the action field is 4 or 6 bits (depending on the type of trigger). Would this mandate a specific WARL behavior that action=1 gets mapped to action=0 in this case (rather than allowing hardware to set action to some other value)? It makes sense to me to map it to 0 rather than something related to trace or a currently reserved encoding but I think that the fact that action is more than one bit must be considered.
Does this mean that the state is allowed to contain action=1 and dmode=0 but that while this situation exists the type field is treated by the logic (for all purposes other than CSR read) as being 0 regardless of the actual value?
Is this a mixture of the two above cases where the state is allowed to contain the illegal state but that while this situation exists the action that the logic uses (for all purposes other than CSR reads) is 0? After reading this, I think that another alternative is to say that |
|
I think the hardware should never be allowed to have an illegal value in a register. It should alter the register value in some way to make the value legal. In this case, attempting to change dmode from 1 to 0 could either leave dmode at 1 or change action to 0. |
Or I think it could set type==0 to disable the trigger so dmode and action are irrelevant. I agree that having illegal values in registers is potentially confusing and goes against the spirit (and probably also the letter) of WARL. I'm OK with any of the 3 proposals (fixing up dmode or action or type). |
|
This is all my fault for bringing it up to begin with. We found a bug in
this area, but looking at the spec, we had no idea what the right way to
fix it was.
The effect you want to disallow is halting on a trigger match when no
external debugger is connected (which is just a hang condition).
There is no wording preventing that setting exactly (it's an ambiguity),
but making the result implementation dependent, but specifying
- what actions are disallowed and
- listing which (combinations) of the above options are allowed
should achieve the desired result.
It might be easier to say that any series of actions that attempt to set
dmode=0, action=1 will cause implementation dependent behavior, including?
limited to? (clearing action, forcing break instead of halt, inhibiting the
write, inhibiting the trigger), whose result not halt based on this trigger.
With my well-worn compliance hat: it's yet another option that may need to
be discovered
…On Fri, Nov 22, 2019 at 11:43 AM Tim Newsome ***@***.***> wrote:
A debugger (or software) should never write an illegal value. Hardware may
choose to protect itself in case it does. I have no strong feelings if that
happens by changing the value as it's written to something legal, or by
ignoring the illegal trigger. Should I just mention both, or just say
something vague like "hardware should ensure not to enter Debug Mode
through a trigger where dmode=0?"
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#501?email_source=notifications&email_token=AHPXVJTXPC3VJHS3KEV7XPTQVAY7XA5CNFSM4JP2ENJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE6VM5Y#issuecomment-557667959>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJV67O76YH2QCME6E23QVAY7XANCNFSM4JP2ENJQ>
.
|
|
- I thought I followed up with a subsequent email that the error code only
makes sense for an external debugger
- yea, I know action a 4bit (or 6bit!) field, but only 1 bit is RW in our
implementation because that's all the spec defines
so in general, map the halt action to break action, which is just
clearing the LSB
- correct, #3 maintains state but disables the trigger; no match can occur
- correct, #4 maintains state, but converts a trigger match halt into a
break
…On Fri, Nov 22, 2019 at 12:08 PM Paul Donahue ***@***.***> wrote:
There are 4 possible scenarios (in what I think is the most complex to the
simplest HW implementation order):
- ignore the write and return an error code
How does an error code get returned on a CSR write?
- clear the action bit
Note that the action field is 4 or 6 bits (depending on the type of
trigger). Would this mandate a specific WARL behavior that action=1 gets
mapped to action=0 in this case (rather than allowing hardware to set
action to some other value)? It makes sense to me to map it to 0 rather
than something related to trace or a currently reserved encoding but I
think that the fact that action is more than one bit must be considered.
- allow the write, but inhibit the trigger
Does this mean that the state is allowed to contain action=1 and dmode=0
but that while this situation exists the type field is treated by the logic
(for all purposes other than CSR read) as being 0 regardless of the actual
value?
- clear the output of the action bit, but not the action bit state
Is this a mixture of the two above cases where the state is allowed to
contain the illegal state but that while this situation exists the action
that the logic uses (for all purposes other than CSR reads) is 0?
After reading this, I think that another alternative is to say that type!=0
&& action==1 && dmode==0 is illegal and let implementations do whatever
WARL behavior they want (clear the type field, clear the action field, set
action to some arbitrary legal value other than 1, keep dmode set, etc.).
I'm not a particularly big fan of this but it does seem more RISC-V-like.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#501?email_source=notifications&email_token=AHPXVJUUX6MOIIUEEWOAG4LQVA35RA5CNFSM4JP2ENJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE6XNWI#issuecomment-557676249>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJT3PMLYGAS7HMPRZDDQVA35RANCNFSM4JP2ENJQ>
.
|
If the external debugger does the CSR write via the program buffer then I think that there's no way to return an error code (unless there's some additional error bit introduced into a CSR and then the debugger would have to sample that after every write but that's ugly and slow).
I'm just saying that if the spec imposes a specific behavior now then we should consider the future and not make a choice that somehow prevents flexibility in defining new actions in the future. We shouldn't paint ourselves into a corner (not that I think that the proposal to clear action does that but I am inviting others to make sure that I'm not wrong). |
|
Paul makes an excellent point that these registers are all WARL, so as Ernie says, the registers should never contain an illegal value. I'll update the PR. |
Hopefully I found some language that makes sense when csrci is used to clear just dmode, as well as writing a brand new value, and only changes action if it's actually necessary.
timsifive
changed the title
Otherwise you might end up in a situation where regular software can
control when to enter debug mode.