Skip to content

Debuggers should know when harts are unavailable. - #520

Merged
Tim Newsome (timsifive) merged 3 commits into
masterfrom
unavailable
Oct 28, 2020
Merged

Debuggers should know when harts are unavailable.#520
Tim Newsome (timsifive) merged 3 commits into
masterfrom
unavailable

Conversation

@timsifive

Copy link
Copy Markdown
Contributor

Make unavail sticky, needing to be acknowledged with new ackunavail bit.

This is a backwards incompatible change. I think it's OK because
implementations that support unavailable harts are rare (I don't know of
any), and the change only means that debuggers must be updated. If
hardware doesn't implement this then everything will still work,
although users might not be notified when harts become unavailable.

Fixes #461.

@RobertChyla

Copy link
Copy Markdown

You may add extra bit 'enable sticky' and then you may have compatible behavior if this new bit is not set. If this bit can be set (by 'new debugger') it will know hart supports that 'sticky-feature'. It is one more bit, but IMO worth it.

@timsifive

Copy link
Copy Markdown
Contributor Author

TODO: Github won't let me assign labels right now, but this one must get the backwards incompatible label.

@RobertChyla

Copy link
Copy Markdown

You may add extra bit 'enable sticky' and then you may have compatible behavior if this new bit is not set. If this bit can be set (by 'new debugger') it will know hart supports that 'sticky-feature'. It is one more bit, but IMO worth it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hart is in 1 of 4 states: non-existent, unavailable, running, or halted. Section 3.4 implies that exactly one of any/allnonexistent, any/allunavail, any/allrunning, any/allhalted is 1 at any particular time, depending on the state of the selected harts.

So, when a hart becomes unavailable, any/allunavail reads as 1 and others read as 0. When the hart then becomes available, any/allunavail remains at 1 until acked. What should any/allrunning and any/allhalted be during this time? Is it true that the hart is in either running or halted state, so one of these should be 1 to indicate that even though any/allunavail is 1? Or does the hart actually remain in unavailable state with both any/allrunning and any/allhalted at 0 until the debugger writes 1 to ackunavail?

@timsifive

Copy link
Copy Markdown
Contributor Author

I imagined that unavail would remain set until it's acknowledged, so it's possible to have unavail set together with running/halted.

I'm puzzling to see if we can make hardware backwards compatible at least without using an extra bit. I'm not very concerned about debuggers needing to be updated because there are relatively few of them and they are relatively easy to change.

@RobertChyla

Copy link
Copy Markdown

Let me comment - maybe it will go through this time.
I can drop debugger compatibility request (extra bit), however some people may be forced to use older debugger (from many reasons).
But it is really important to handle situation, when hart is leaving 'unavail' state for very short period of time. Maybe stop request should force transition from 'unavail' into 'stopped' and if core/hart is in 'stopped' state some SoC may prevent to put it into 'unavail' (by blocking clock/power gating)?
This may also life of simple debuggers when core/hart will never 'disappear' once stopped.
Again - forgive me lack of precise terms - my knowledge of exact bits faded away.

@timsifive

Copy link
Copy Markdown
Contributor Author

This proposal explicitly addresses harts briefly becoming unavailable. In the "Files changed" tab above you can see the exact language I wrote.

@timsifive

Copy link
Copy Markdown
Contributor Author

This doesn't really address Ernie Edgar (@ernie-sifive) 's feedback that the intro to Section 3.4 implies only one of these bits is set. We don't even really mention these bits, and I'm struggling with a concise way to describe them: something that's helpful and not even more confusing.

Maybe what's needed is a state diagram/table...

Make unavail sticky, needing to be acknowledged with new ackunavail bit.

This is a backwards incompatible change. I think it's OK because
implementations that support unavailable harts are rare (I don't know of
any), and the change only means that debuggers must be updated. If
hardware doesn't implement this then everything will still work,
although users might not be notified when harts become unavailable.

Fixes #461.
Every other way to ensure hardware backwards compatibility was getting
too convoluted.
Keep definition of the bits in the register definition, and definition
of states in the DM section.
@timsifive

Copy link
Copy Markdown
Contributor Author

I've tweaked this a little, but I rescind my earlier comment about things not being clear. As it is, the states are defined in Section 3.4, while the description of what exactly the bits do is described in the register definition.

Thoughts?

@pdonahue-ventana

Copy link
Copy Markdown
Collaborator

This seems to address #461 where the debugger can know if a hart unexpectedly became unavailable. For a complete solution, can there also be a mechanism to address #519 to prevent this from happening to start with (similar to ARM's CORENPDRQ)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

behavior when harts become unavailable during an abstract command

4 participants